Review definition of DATA for SQL files in the Makefile

[michaelpq]

HI all,

We do the following in the Makefile: DATA = pg_hint_plan--*.sql

This rule exists since 31b4a22, but I think that this is heavily error-prone, because we could remove files unrelated to the version we are working on if for example mixing paths with different major versions. I think that we should revisit this decision, and explicitly list directly all the SQL files we have to install. This is surely a nit more maintenance burden, but I think that's for the best at the end.

Thoughts?

[rjuju]

IIUC the current approach is to not provide upgrade path and only provide the SQL for the current version. This seems a bit troublesome since you can save hints in the hint_plan.hints. So I'm fine with explicitly stating the extension SQL files, but we should consider providing upgrade script (even if they will likely be empty most of the time).

[michaelpq]

IIUC the current approach is to not provide upgrade path and only provide the SQL for the current version. This seems a bit troublesome since you can save hints in the hint_plan.hints. So I'm fine with explicitly stating the extension SQL files, but we should consider providing upgrade script (even if they will likely be empty most of the time).

Are you referring to the SQL files to be able to jump from 1.3.X to 1.4? If that's what you mean, I definitely agree. There should be a minimal set of regression tests to cover the changes, as well.

[rjuju]

Yes that's what I meant!

[michaelpq]

Yes that's what I meant!

Okay, I'll send a pull request to show the path for HEAD, but we need that for all the stable branches down to 1.3.5, roughly. Just to be sure: the oldest full SQL script just needs to match with the oldest version we can still upgrade from in the oldest stable branch, aka 1.3.5, right?

I mean that HEAD needs to be changed so as we have 1.3.5 -> 1.3.5--1.3.6 -> 1.3.6--1.3.7 -> 1.4 -> 1.5. This way, if we add a 1.4--1.4.1, we'll still be able to plug it in the HEAD chain with 1.5 as latest. Do we agree? If you have a different vision of things, feel free to dump it here.

[michaelpq]

I have hacked on that, and sent a pull request with what would be the heaviest approach possible: https://github.com/ossc-db/pg_hint_plan/pull/111

As the SQL scripts only include the hint table, we could get away with just the following bits in each upgrade script:

SELECT pg_catalog.pg_extension_config_dump('hint_plan.hints','');
SELECT pg_catalog.pg_extension_config_dump('hint_plan.hints_id_seq','');
GRANT SELECT ON hint_plan.hints TO PUBLIC;
GRANT USAGE ON SCHEMA hint_plan TO PUBLIC;

I have included INEs to make sure that we always have the expected table definition in all these upgrade paths and keep the existing hints so as the code would basically work for any versions used based on the origin (well, that would not work if one changes the attributes of the table anyway). Perhaps that's me worrying too much? At this stage, my patch would involve only the removal of code, so that's simple enough to switch from one approach to the other. Thoughts?

[horiguti]

I didn't provide the inter-major-version upgrade paths as it is treated as the same with the other extensions maintained together that don't have on-disk data compatibility but this one has that nature as you wrote. I don't assume that table hints are used widely. So thanks for working on that.

we could get away with just the following bits in each upgrade script:

I don't think the table ought to exist when an upgrading script runs. (Otherwise the user even didn't CREATE EXTIENSION in the first place :p) So I think that the four lines are enough. and the INEs (means the whole CREATE TABLEs?) are a bit too much.

[michaelpq]

I don't think the table ought to exist when an upgrading script runs. (Otherwise the user even didn't CREATE EXTIENSION in the first place :p) So I think that the four lines are enough. and the INEs (means the whole CREATE TABLEs?) are a bit too much.

I was not sure if it was OK to assume that the hint table always existed when the extension was previously created. As it sounds here, you are confirming that this is the case, hence I agree that keeping the table definitions in the oldest script while providing the two SELECTs and the two GRANTs in all the incremental scripts should be enough.

[horiguti]

Are you considering the case where a user dropped the table after CREATE EXTENSION? I can imagine the reverse (a user creates it without running CREATE EXTENSION) but..

[michaelpq]

Are you considering the case where a user dropped the table after CREATE EXTENSION?

Nah, I was just wondering if one was able to do CREATE EXTENSION without this table getting created, like if there were some different objects created in the project's history. Looks like that's not the case as far as I have checked.

[horiguti]

How one can ALTER EXTENSION UPDATE TO without preceding CREATE EXTENSION? In that case, even ALTER EXTESION is not needed. Just replacing .so will work.

[michaelpq]

One could drop the table with ALTER EXTENSION. But that does not fly as something we need to support :)