Use CitusDB SELECT logic by default within CitusDB

[jasonmp85]

The use_citusdb_select_logic configuration parameter defaults false, which is causing confusion now that pg_shard sees and understands all CitusDB metadata. This detects CitusDB at build time and defaults the configuration to true when pg_shard is built against CitusDB.

Resolves #132

[sumedhpathak]

Can this change go in the .sql file itself? How is the order of these things being called? We already have logic there to determine if we are building against Citus, and take different actions based on that. Would we be able to set this variable there?

[jasonmp85]

Can this change go in the .sql file itself?

The SQL file is an installation script, so it runs once (at installation).

I could ALTER SYSTEM from within SQL to write out a value to be used within the cluster hosting pg_shard, but:

• That's not in PostgreSQL 9.3
• It requires superuser permissions
• It just writes out a config file; the underlying default (in the presence of no config) is the same

At any rate, custom GUCs are defined within _PG_init, which runs when the extension is loaded, so to change the default value of this GUC we need to detect CitusDB there somehow.

How is the order of these things being called?

I'm not 100% sure what this means, but the argument I changed here changes the underlying default value for the boolean GUC it's declaring. It runs during _PG_init, which runs (because we favor shared_preload_libraries) in the frontend, before any backend process starts. This severely limits what is possible within _PG_init (and is why in a previous review where I needed to detect CitusDB within _PG_init I had to detect a CitusDB-specific GUC. This time I realized there are some #defines I could use instead).

We already have logic there to determine if we are building against Citus, and take different actions based on that. Would we be able to set this variable there?

That isn't if we're building against CitusDB. By the time that code runs, the extension is built and has been put in place (by a make install). The .sql file is for CREATE EXTENSION. So at that point the library (.so or whatever) is built and the default value is "baked in".

Bigger picture: all the ways of changing the "default" from a higher level (configuration, ALTER SYSTEM, ALTER DATABASE, SET) have the fundamental limitation that—if the user removes them—the "default as defined in the C code" will be used. The "default as defined by the C code" is set by the call in _PG_init, so we need to do the detection there. And because we suggest people preload our library, _PG_init will run in a frontend, limiting our options.

If you're worried about duplication (or two ways of detecting CitusDB), I can wrap the #ifdef block in a function (BuiltAgainstCitusDB) and call it from all the places we need to detect CitusDB.

[jasonmp85]

@sumedhpathak if you're OK with this approach, I can kick this over to @onderkalaci to review.

[sumedhpathak]

@jasonmp85 I'm ok with the approach. Let's pass to @onderkalaci for a review, and also test it a bit thoroughly as well.

[jasonmp85]

Cleaned up and ready for review by @onderkalaci.

[onderkalaci]

Hey @jasonmp85,

Important Note: The below problem happened because I've forgotten to update preload_shared_libraries config. But, I haven't deleted, since anyone may fell into the same situation. So, it is good to remember that :))

Before reviewing this PR, I realized a bug related to the same config variable (well, it seems it is a bug). I note it here. Do you think it is a bug? Or am I missing anything?

I followed the steps below (on both master and this branch:):

• Compile&install pg_shard against CitusDB:

  sudo PATH=/opt/citusdb/4.0/bin/:$PATH make install

• Connect to the master

  /opt/citusdb/4.0/bin/psql -d  postgres

• create pg_shard

  CREATE EXTENSION pg_shard

• See the current value of the flag:

  postgres=# show pg_shard.use_citusdb_select_logic ;
  pg_shard.use_citusdb_select_logic 
  -----------------------------------
  on
  (1 row)

• Disconnect from the terminal: ctrl+c

• Reconnect and see the current value again:

  postgres=# show pg_shard.use_citusdb_select_logic ;
  ERROR:  unrecognized configuration parameter "pg_shard.use_citusdb_select_logic"

[onderkalaci]

Hey @jasonmp85,

I reviewed and tested the changes, all looks fine. I understand from what you wrote that there is no other way to do it without compile time detection. But, maybe in another PR, we may consider to do what you suggested to make the detection be the same for all places: If you're worried about duplication (or two ways of detecting CitusDB), I can wrap the #ifdef block in a function (BuiltAgainstCitusDB) and call it from all the places we need to detect CitusDB.

There is only one question in my mind. Assume we follow the steps below:

• Compile pg_shard against Postgres
• Use that pg_shard binary with CitusDB
• Now, master_create_worker_shards call crashes.

I was trying to experiment such a corner case. But, the above steps fail on the master branch as well. I think we shouldn't be taking care of this, because we shouldn't be supporting this at all. What do you think?

If you also agree with the above, I think we're ready to :shipit:

[jasonmp85]

Before reviewing this PR, I realized a bug related to the same config variable (well, it seems it is a bug). I note it here. Do you think it is a bug? Or am I missing anything?

The other day I noticed that the Bi-Directional Replication plugin raises an error when it's not in preload_shared_libraries. Maybe we should do something similar.

At any rate, the issue you experienced is best solved independently, probably as part of #93.

[jasonmp85]

I was trying to experiment such a corner case. But, the above steps fail on the master branch as well. I think we shouldn't be taking care of this, because we shouldn't be supporting this at all. What do you think?

Yes, we definitely don't want to support building an extension in PostgreSQL and trying to run it in CitusDB. Even without this change, other parts of compilation are dependent upon the version of PostgreSQL being used for compilation (such as compiling the "right" version of ruleutils. The older version might work in a newer PostgreSQL version, but perhaps some of the in-memory representation of nodes will have changed between versions and could result in subtle bugs).

Also, since this pull request is against master (to get this into a release ASAP), I'm extremely disinclined to solve anything but the issue at hand before a merge.

Gonna merge and release v1.2.2.