Обсуждение: Re: Adding SHOW CREATE TABLE
On Sun, May 14, 2023 at 2:20 AM Kirk Wolak <wolakk@gmail.com> wrote:
On Sat, May 13, 2023 at 3:34 PM Jeremy Smith <jeremy@musicsmith.net> wrote:On Sat, May 13, 2023, 3:25 AM Kirk Wolak <wolakk@gmail.com> wrote:Does this imply SQL SYNTAX like:
SHOW CREATE TABLE <table_name>[ INCLUDING { ALL | INDEXES | SEQUENCES | ??? }][EXCLUDING { PK | FK | COMMENTS | STORAGE | } ][FOR {V11 | V12 | V13 | V14 | V15 }] ???Personally, I would expect a function, like pg_get_tabledef(oid), to match the other pg_get_*def functions instead of overloading SHOW. To me, this also argues that we shouldn't include indexes because we already have a pg_get_indexdef function.-Jeremy+1In fact, making it a function will make my life easier for testing, that's for certain. I don't need to involve the parser,etc. Others can help with that after the function works.Thanks for the suggestion!
I am moving this over to the Hackers Group.
My approach for now is to develop this as the \st command.
After reviewing the code/output from the 3 sources (psql, fdw, and pg_dump). This trivializes the approach,
and requires the smallest set of changes (psql is already close with existing queries, etc).
And frankly, I would rather have an \st feature that handles 99% of the use cases then go 15yrs waiting for a perfect solution.
Once this works well for the group. Then, IMO, that would be the time to discuss moving it.
Support Or Objections Appreciated.
Kirk...
Greetings, * Kirk Wolak (wolakk@gmail.com) wrote: > My approach for now is to develop this as the \st command. > After reviewing the code/output from the 3 sources (psql, fdw, and > pg_dump). This trivializes the approach, > and requires the smallest set of changes (psql is already close with > existing queries, etc). > > And frankly, I would rather have an \st feature that handles 99% of the use > cases then go 15yrs waiting for a perfect solution. > Once this works well for the group. Then, IMO, that would be the time to > discuss moving it. Having this only available via psql seems like the least desirable option as then it wouldn't be available to any other callers.. Having it in libpq, on the other hand, would make it available to psql as well as any other utility, library, or language / driver which uses libpq, including pg_dump.. Using libpq would also make sense from the perspective that libpq can be used to connect to a number of different major versions of PG and this could work work for them all in much the way that pg_dump does. The downside with this apporach is that drivers which don't use libpq (eg: JDBC) would have to re-implement this if they wanted to keep feature parity with libpq.. Thanks, Stephen
Вложения
On 2023-05-18 Th 19:53, Stephen Frost wrote:
Greetings, * Kirk Wolak (wolakk@gmail.com) wrote:My approach for now is to develop this as the \st command. After reviewing the code/output from the 3 sources (psql, fdw, and pg_dump). This trivializes the approach, and requires the smallest set of changes (psql is already close with existing queries, etc). And frankly, I would rather have an \st feature that handles 99% of the use cases then go 15yrs waiting for a perfect solution. Once this works well for the group. Then, IMO, that would be the time to discuss moving it.Having this only available via psql seems like the least desirable option as then it wouldn't be available to any other callers.. Having it in libpq, on the other hand, would make it available to psql as well as any other utility, library, or language / driver which uses libpq, including pg_dump.. Using libpq would also make sense from the perspective that libpq can be used to connect to a number of different major versions of PG and this could work work for them all in much the way that pg_dump does. The downside with this apporach is that drivers which don't use libpq (eg: JDBC) would have to re-implement this if they wanted to keep feature parity with libpq..
I think the ONLY place we should have this is in server side functions. More than ten years ago I did some work in this area (see below), but it's one of those things that have been on my ever growing personal TODO list
See <https://bitbucket.org/adunstan/retailddl/src/master/> and <https://www.youtube.com/watch?v=fBarFKOL3SI>
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
I think the ONLY place we should have this is in server side functions. More than ten years ago I did some work in this area (see below), but it's one of those things that have been on my ever growing personal TODO list
See <https://bitbucket.org/adunstan/retailddl/src/master/> and <https://www.youtube.com/watch?v=fBarFKOL3SI>
Some additional backstory, as this has come up before...
A direction discussion of a previous SHOW CREATE effort:
https://www.postgresql.org/message-id/20190705163203.GD24679@fetter.org
Other databases' implementations of SHOW CREATE came up in this discussion as well:
https://www.postgresql.org/message-id/CADkLM=fxfsrHASKk_bY_A4uomJ1Te5MfGgD_rwwQfV8wP68ewg@mail.gmail.com
Clearly, there is customer demand for something like this, and has been for a long time.
A direction discussion of a previous SHOW CREATE effort:
https://www.postgresql.org/message-id/20190705163203.GD24679@fetter.org
Other databases' implementations of SHOW CREATE came up in this discussion as well:
https://www.postgresql.org/message-id/CADkLM=fxfsrHASKk_bY_A4uomJ1Te5MfGgD_rwwQfV8wP68ewg@mail.gmail.com
Clearly, there is customer demand for something like this, and has been for a long time.
On Fri, 2023-05-19 at 13:08 -0400, Andrew Dunstan wrote:
> On 2023-05-18 Th 19:53, Stephen Frost wrote:
> > * Kirk Wolak (wolakk@gmail.com) wrote:
> > > My approach for now is to develop this as the \st command.
> > > After reviewing the code/output from the 3 sources (psql, fdw, and
> > > pg_dump).
> >
> > Having this only available via psql seems like the least desirable
> > option as then it wouldn't be available to any other callers..
> >
> > Having it in libpq, on the other hand, would make it available to psql
> > as well as any other utility, library, or language / driver which uses
> > libpq, including pg_dump..
>
> I think the ONLY place we should have this is in server side functions.
+1
A function "pg_get_tabledef" would blend nicely into the following list:
\df pg_get_*def
List of functions
Schema │ Name │ Result data type │ Argument data types │ Type
════════════╪════════════════════════════════╪══════════════════╪═══════════════════════╪══════
pg_catalog │ pg_get_constraintdef │ text │ oid │ func
pg_catalog │ pg_get_constraintdef │ text │ oid, boolean │ func
pg_catalog │ pg_get_functiondef │ text │ oid │ func
pg_catalog │ pg_get_indexdef │ text │ oid │ func
pg_catalog │ pg_get_indexdef │ text │ oid, integer, boolean │ func
pg_catalog │ pg_get_partition_constraintdef │ text │ oid │ func
pg_catalog │ pg_get_partkeydef │ text │ oid │ func
pg_catalog │ pg_get_ruledef │ text │ oid │ func
pg_catalog │ pg_get_ruledef │ text │ oid, boolean │ func
pg_catalog │ pg_get_statisticsobjdef │ text │ oid │ func
pg_catalog │ pg_get_triggerdef │ text │ oid │ func
pg_catalog │ pg_get_triggerdef │ text │ oid, boolean │ func
pg_catalog │ pg_get_viewdef │ text │ oid │ func
pg_catalog │ pg_get_viewdef │ text │ oid, boolean │ func
pg_catalog │ pg_get_viewdef │ text │ oid, integer │ func
pg_catalog │ pg_get_viewdef │ text │ text │ func
pg_catalog │ pg_get_viewdef │ text │ text, boolean │ func
(17 rows)
A server function can be conveniently called from any client code.
Yours,
Laurenz Albe
On 19/05/2023 19:08, Andrew Dunstan wrote: > I think the ONLY place we should have this is in server side > functions. More than ten years ago I did some work in this area (see > below), but it's one of those things that have been on my ever growing > personal TODO list > > See <https://bitbucket.org/adunstan/retailddl/src/master/> and > <https://www.youtube.com/watch?v=fBarFKOL3SI> > I have my own implementation of SHOW CREATE as a ddlx extension available at https://github.com/lacanoid/pgddl Far from perfect but getting better and it seems good enough to be used by some people... Ž.
Greetings, * Laurenz Albe (laurenz.albe@cybertec.at) wrote: > On Fri, 2023-05-19 at 13:08 -0400, Andrew Dunstan wrote: > > On 2023-05-18 Th 19:53, Stephen Frost wrote: > > > * Kirk Wolak (wolakk@gmail.com) wrote: > > > > My approach for now is to develop this as the \st command. > > > > After reviewing the code/output from the 3 sources (psql, fdw, and > > > > pg_dump). > > > > > > Having this only available via psql seems like the least desirable > > > option as then it wouldn't be available to any other callers.. > > > > > > Having it in libpq, on the other hand, would make it available to psql > > > as well as any other utility, library, or language / driver which uses > > > libpq, including pg_dump.. > > > > I think the ONLY place we should have this is in server side functions. > > +1 ... but it already exists in pg_dump, so I'm unclear why folks seem to be pushing to have a duplicate of it in core? We certainly can't remove it from pg_dump even if we add it to core because pg_dump has to understand the changes between major versions. > A function "pg_get_tabledef" would blend nicely into the following list: > > \df pg_get_*def > List of functions > Schema │ Name │ Result data type │ Argument data types │ Type > ════════════╪════════════════════════════════╪══════════════════╪═══════════════════════╪══════ > pg_catalog │ pg_get_constraintdef │ text │ oid │ func > pg_catalog │ pg_get_constraintdef │ text │ oid, boolean │ func > pg_catalog │ pg_get_functiondef │ text │ oid │ func > pg_catalog │ pg_get_indexdef │ text │ oid │ func > pg_catalog │ pg_get_indexdef │ text │ oid, integer, boolean │ func > pg_catalog │ pg_get_partition_constraintdef │ text │ oid │ func > pg_catalog │ pg_get_partkeydef │ text │ oid │ func > pg_catalog │ pg_get_ruledef │ text │ oid │ func > pg_catalog │ pg_get_ruledef │ text │ oid, boolean │ func > pg_catalog │ pg_get_statisticsobjdef │ text │ oid │ func > pg_catalog │ pg_get_triggerdef │ text │ oid │ func > pg_catalog │ pg_get_triggerdef │ text │ oid, boolean │ func > pg_catalog │ pg_get_viewdef │ text │ oid │ func > pg_catalog │ pg_get_viewdef │ text │ oid, boolean │ func > pg_catalog │ pg_get_viewdef │ text │ oid, integer │ func > pg_catalog │ pg_get_viewdef │ text │ text │ func > pg_catalog │ pg_get_viewdef │ text │ text, boolean │ func > (17 rows) > > > A server function can be conveniently called from any client code. Clearly any client using libpq can conveniently call code which is in libpq. Thanks, Stephen
Вложения
On Sat, May 20, 2023 at 10:26 AM Stephen Frost <sfrost@snowman.net> wrote:
> A server function can be conveniently called from any client code.
Clearly any client using libpq can conveniently call code which is in
libpq.
Clearly there are clients that don't use libpq. JDBC comes to mind.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Sat, May 20, 2023 at 10:26 AM Stephen Frost <sfrost@snowman.net> wrote:
>> Clearly any client using libpq can conveniently call code which is in
>> libpq.
> Clearly there are clients that don't use libpq. JDBC comes to mind.
Yeah. I'm also rather concerned about the bloat factor; it's
hardly unlikely that this line of development would double the
size of libpq, to add functionality that only a small minority
of applications would use. A client-side implementation also has
no choice but to cope with multiple server versions, and to
figure out what it's going to do about a too-new server.
Up to now we broke compatibility between libpq and server maybe
once every couple decades, but it'd be once a year for this.
regards, tom lane
Greetings,
On Sat, May 20, 2023 at 13:32 David G. Johnston <david.g.johnston@gmail.com> wrote:
On Sat, May 20, 2023 at 10:26 AM Stephen Frost <sfrost@snowman.net> wrote:> A server function can be conveniently called from any client code.
Clearly any client using libpq can conveniently call code which is in
libpq.Clearly there are clients that don't use libpq. JDBC comes to mind.
Indeed … as I mentioned up-thread already.
Are we saying that we want this to be available server side, and largely duplicated, specifically to cater to non-libpq users? I’ll put out there, again, the idea that perhaps we put it into the common library then and make it available via both libpq and as a server side function ..?
We also have similar code in postgres_fdw.. ideally, imv anyway, we’d not end up with three copies of it.
Thanks,
Stephen
On Sat, May 20, 2023 at 2:33 PM Stephen Frost <sfrost@snowman.net> wrote:
Greetings,On Sat, May 20, 2023 at 13:32 David G. Johnston <david.g.johnston@gmail.com> wrote:On Sat, May 20, 2023 at 10:26 AM Stephen Frost <sfrost@snowman.net> wrote:> A server function can be conveniently called from any client code.
Clearly any client using libpq can conveniently call code which is in
libpq.Clearly there are clients that don't use libpq. JDBC comes to mind.Indeed … as I mentioned up-thread already.Are we saying that we want this to be available server side, and largely duplicated, specifically to cater to non-libpq users? I’ll put out there, again, the idea that perhaps we put it into the common library then and make it available via both libpq and as a server side function ..?We also have similar code in postgres_fdw.. ideally, imv anyway, we’d not end up with three copies of it.Thanks,Stephen
First, as the person chasing this down, and a JDBC user, I really would prefer pg_get_tabledef() as Laurenz mentioned.
Next, I have reviewed all 3 implementations (pg_dump [most appropriate], psql \d (very similar), and the FDW which is "way off",
since it actually focuses on "CREATE FOREIGN TABLE" exclusively, and already fails to handle many pieces not required in
creating a "real" table, as it creates a "reflection" of table.
I am using pg_dump as my source of truth. But I noticed it does not create "TEMPORARY" tables with that syntax.
[Leading to a question on mutating the pg_temp_# schema name back to pg_temp. or just stripping it, in favor of the TEMPORARY]
I was surprised to see ~ 2,000 lines of code in the FDW and in psql... Whereas pg_dump is shorter because it gets more detailed
table information in a structure passed in.
I would love to leverage existing code, in the end. But I want to take my time on this, and become intimate with the details.
Each of the above 3 approaches have different goals. And I would prefer the lowest risk:reward possible, and the least expensive
maintenance. Having it run server side hides a ton of details, and as Tom pointed out, obviates DDL versioning control for other server versions.
Thanks for the references to the old discussions. I have queued them up to review.
Kirk...
On Fri, May 19, 2023 at 1:08 PM Andrew Dunstan <andrew@dunslane.net> wrote:
I think the ONLY place we should have this is in server side functions. More than ten years ago I did some work in this area (see below), but it's one of those things that have been on my ever growing personal TODO list
See <https://bitbucket.org/adunstan/retailddl/src/master/> and <https://www.youtube.com/watch?v=fBarFKOL3SI>
Andrew,
Thanks for sharing that. I reviewed your code. 10yrs, clearly it's not working (as-is, but close), something interesting about the
structure you ended up in. You check the type of the object and redirect accordingly at the top level. Hmmm...
What I liked was that each type gets handled (I was focused on "table"), but I realized similarities.
I don't know what the group would think, but I like the thought of calling this, and having it "Correct" to call the appropriate function.
But not sure it will stand. It does make obvious that some of these should be spun out as "pg_get_typedef"..
pg_get_typedef
pg_get_domaindef
pg_get_sequencedef
Finally, since you started this a while back, part of me is "leaning" towards a function:
pg_get_columndef
pg_get_columndef
Which returns a properly formatted column for a table, type, or domain? (one of the reasons for this, is that this is
the function with the highest probability to change, and potentially the easiest to share reusability).
Finally, I am curious about your opinion. I noticed you used the internal pg_ tables, versus the information_schema...
I am *thinking* that the information_schema will be more stable over time... Thoughts?
Thank you for sharing your thoughts...
Kirk...
po 22. 5. 2023 v 7:19 odesílatel Kirk Wolak <wolakk@gmail.com> napsal:
On Fri, May 19, 2023 at 1:08 PM Andrew Dunstan <andrew@dunslane.net> wrote:I think the ONLY place we should have this is in server side functions. More than ten years ago I did some work in this area (see below), but it's one of those things that have been on my ever growing personal TODO list
See <https://bitbucket.org/adunstan/retailddl/src/master/> and <https://www.youtube.com/watch?v=fBarFKOL3SI>
Andrew,Thanks for sharing that. I reviewed your code. 10yrs, clearly it's not working (as-is, but close), something interesting about thestructure you ended up in. You check the type of the object and redirect accordingly at the top level. Hmmm...What I liked was that each type gets handled (I was focused on "table"), but I realized similarities.I don't know what the group would think, but I like the thought of calling this, and having it "Correct" to call the appropriate function.But not sure it will stand. It does make obvious that some of these should be spun out as "pg_get_typedef"..pg_get_typedefpg_get_domaindefpg_get_sequencedefFinally, since you started this a while back, part of me is "leaning" towards a function:
pg_get_columndefWhich returns a properly formatted column for a table, type, or domain? (one of the reasons for this, is that this isthe function with the highest probability to change, and potentially the easiest to share reusability).Finally, I am curious about your opinion. I noticed you used the internal pg_ tables, versus the information_schema...I am *thinking* that the information_schema will be more stable over time... Thoughts?
I think inside the core, the information schema is never used. And there was a performance issue (fixed in PostgreSQL 12), that blocked index usage.
Regards
Pavel
Thank you for sharing your thoughts...Kirk...
On 2023-05-22 Mo 05:24, Pavel Stehule wrote:
po 22. 5. 2023 v 7:19 odesílatel Kirk Wolak <wolakk@gmail.com> napsal:On Fri, May 19, 2023 at 1:08 PM Andrew Dunstan <andrew@dunslane.net> wrote:I think the ONLY place we should have this is in server side functions. More than ten years ago I did some work in this area (see below), but it's one of those things that have been on my ever growing personal TODO list
See <https://bitbucket.org/adunstan/retailddl/src/master/> and <https://www.youtube.com/watch?v=fBarFKOL3SI>
Andrew,Thanks for sharing that. I reviewed your code. 10yrs, clearly it's not working (as-is, but close), something interesting about thestructure you ended up in. You check the type of the object and redirect accordingly at the top level. Hmmm...What I liked was that each type gets handled (I was focused on "table"), but I realized similarities.I don't know what the group would think, but I like the thought of calling this, and having it "Correct" to call the appropriate function.But not sure it will stand. It does make obvious that some of these should be spun out as "pg_get_typedef"..pg_get_typedefpg_get_domaindefpg_get_sequencedefFinally, since you started this a while back, part of me is "leaning" towards a function:
pg_get_columndefWhich returns a properly formatted column for a table, type, or domain? (one of the reasons for this, is that this isthe function with the highest probability to change, and potentially the easiest to share reusability).Finally, I am curious about your opinion. I noticed you used the internal pg_ tables, versus the information_schema...I am *thinking* that the information_schema will be more stable over time... Thoughts?I think inside the core, the information schema is never used. And there was a performance issue (fixed in PostgreSQL 12), that blocked index usage.
A performant server side set of functions would be written in C and follow the patterns in ruleutils.c.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Mon, 22 May 2023 at 13:52, Andrew Dunstan <andrew@dunslane.net> wrote: > A performant server side set of functions would be written in C and follow the patterns in ruleutils.c. We have lots of DDL ruleutils in our Citus codebase: https://github.com/citusdata/citus/blob/main/src/backend/distributed/deparser/citus_ruleutils.c I'm pretty sure we'd be happy to upstream those if that meant, we wouldn't have to update them for every postgres release. We also have the master_get_table_ddl_events UDF, which does what SHOW CREATE TABLE would do.
On 2023-05-25 Th 09:23, Jelte Fennema wrote:
On Mon, 22 May 2023 at 13:52, Andrew Dunstan <andrew@dunslane.net> wrote:A performant server side set of functions would be written in C and follow the patterns in ruleutils.c.We have lots of DDL ruleutils in our Citus codebase: https://github.com/citusdata/citus/blob/main/src/backend/distributed/deparser/citus_ruleutils.c I'm pretty sure we'd be happy to upstream those if that meant, we wouldn't have to update them for every postgres release. We also have the master_get_table_ddl_events UDF, which does what SHOW CREATE TABLE would do.
Sounds promising. I'd love to see a patch.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, May 25, 2023 at 9:23 AM Jelte Fennema <postgres@jeltef.nl> wrote:
On Mon, 22 May 2023 at 13:52, Andrew Dunstan <andrew@dunslane.net> wrote:
> A performant server side set of functions would be written in C and follow the patterns in ruleutils.c.
We have lots of DDL ruleutils in our Citus codebase:
https://github.com/citusdata/citus/blob/main/src/backend/distributed/deparser/citus_ruleutils.c
I'm pretty sure we'd be happy to upstream those if that meant, we
wouldn't have to update them for every postgres release.
We also have the master_get_table_ddl_events UDF, which does what SHOW
CREATE TABLE would do.
Jelte, this looks promising, although it is a radically different approach (Querying from it to get the details).
I was just getting ready to write up a bit of an RFC... On the following approach...
I have been trying to determine how to "focus" this effort to move it forward. Here is where I am at:
1) It should be 100% server side (and psql \st would only work by calling the server side code, if it was there)
In reviewing... This simplifies the implementation to the current version of PG DDL being generated.
Also, as others have mentioned, it should be C based code, and use only the internal tables.
2) Since pg_get_{ triggerdef | indexdef | constraintdef } already exists, I was strongly recommending to not include those.
-- Although including the inlined constraints would be fine by me (potentially a boolean to turn it off?)
3) Then focusing the reloptions WITH (%s)
It appears CITUS code handles ALL of this on a cursory review!
The ONLY thing I did not see was "CREATE TEMPORARY " syntax? If you did this on a TEMP table,
does it generate normal table syntax or TEMPORARY TABLE syntax???
So, from my take... This is a great example of solving the problem with existing "Production Quality" Code...
So, from my take... This is a great example of solving the problem with existing "Production Quality" Code...
I like it...
Can this get turned into a Patch? Were you offering this code up for others (me?) to pull, and work into a patch?
[If I do the patch, I am not sure it gives you the value of reducing what CITUS has to maintain. But it dawns on
me that you might be pushing a much bigger patch... But I would take that, as I think there is other value in there]
Others???
Thanks,
Kirk...
PS: It dawned on me that if pg_dump had used server side code to generate its DDL, its complexity would drop.
On 2023-06-01 Th 12:57, Kirk Wolak wrote:
PS: It dawned on me that if pg_dump had used server side code to generate its DDL, its complexity would drop.
Maybe, that remains to be seen. pg_dump needs to produce SQL that is suitable for the target database, not the source database.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, Jun 1, 2023 at 3:13 PM Andrew Dunstan <andrew@dunslane.net> wrote:
On 2023-06-01 Th 12:57, Kirk Wolak wrote:
PS: It dawned on me that if pg_dump had used server side code to generate its DDL, its complexity would drop.Maybe, that remains to be seen. pg_dump needs to produce SQL that is suitable for the target database, not the source database.
First, pg_dump has some special needs in addressing how it creates tables, to be able to load the data BEFORE indexing, and constraining (lest you have to struggle with dependencies of FKs, etc etc)...
But version checking is interesting... Because I found no way to tell pg_dump what DB to target. The code I did see was checking what server options were available. (I clearly could have missed something)... But exactly how would that work? All it knows is who it is (pg_dump V14, for example. So the best case is that it ignores new things that would be in V15, V16 because it doesn't even know what to check for or what to do). Which, to me, makes it more of a side-effect than a feature, no?
And that if it used a server side solution, it gets an interesting "super power". In that even pg_dump V20 could dump a V21 db with V21 syntax enhancements. (I cannot say if that is desirable or not as I lack 20yrs of history here). Finally, the thoughts of implementing this change at this point, when it would impact anyone using the NEW version to dump an OLD version without the server side support... So that migration may never happen. Or only after the only supported DBs have the required server side functions...
The feedback is welcome...
On 2023-06-01 Th 16:39, Kirk Wolak wrote:
On Thu, Jun 1, 2023 at 3:13 PM Andrew Dunstan <andrew@dunslane.net> wrote:On 2023-06-01 Th 12:57, Kirk Wolak wrote:
PS: It dawned on me that if pg_dump had used server side code to generate its DDL, its complexity would drop.Maybe, that remains to be seen. pg_dump needs to produce SQL that is suitable for the target database, not the source database.
First, pg_dump has some special needs in addressing how it creates tables, to be able to load the data BEFORE indexing, and constraining (lest you have to struggle with dependencies of FKs, etc etc)...But version checking is interesting... Because I found no way to tell pg_dump what DB to target.
The target version is implicitly the version it's built from.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Thu, Jun 1, 2023 at 4:46 PM Andrew Dunstan <andrew@dunslane.net> wrote:
On 2023-06-01 Th 16:39, Kirk Wolak wrote:
On Thu, Jun 1, 2023 at 3:13 PM Andrew Dunstan <andrew@dunslane.net> wrote:On 2023-06-01 Th 12:57, Kirk Wolak wrote:
PS: It dawned on me that if pg_dump had used server side code to generate its DDL, its complexity would drop.Maybe, that remains to be seen. pg_dump needs to produce SQL that is suitable for the target database, not the source database.
First, pg_dump has some special needs in addressing how it creates tables, to be able to load the data BEFORE indexing, and constraining (lest you have to struggle with dependencies of FKs, etc etc)...But version checking is interesting... Because I found no way to tell pg_dump what DB to target.The target version is implicitly the version it's built from.
Andrew,
Thank you. Someone else confirmed for me that the code is designed to create accurate DDL for the pg_dump version.
So, for example, WITH OIDs are not included with later versions of pg_dump, even though they are hitting a server with them.
Great to know.
I like that CITUS offered up something. I think that might be the current best approach. It's a win-win. They get something,
we get something.
Kirk...
On Thu, 1 Jun 2023 at 18:57, Kirk Wolak <wolakk@gmail.com> wrote: > Can this get turned into a Patch? Were you offering this code up for others (me?) to pull, and work into a patch? > [If I do the patch, I am not sure it gives you the value of reducing what CITUS has to maintain. But it dawns on > me that you might be pushing a much bigger patch... But I would take that, as I think there is other value in there] Attached is a patch which adds the relevant functions from Citus to Postgres (it compiles without warnings on my machine). I checked with a few others on the Citus team at Microsoft and everyone thought that upstreaming this was a good idea, because it's quite a bit of work to update with every postgres release. To set expectations though, I don't really have time to work on this patch. So if you can take it over from here that would be great. The patch only contains the C functions which generate SQL based on some oids. The wrappers such as the master_get_table_ddl_events function were too hard for me to pull out of Citus code, because they integrated a lot with other pieces. But the bulk of the logic is in the functions in this patch. Essentially all that master_get_table_ddl_events does is call the functions in this patch in the right order. > The ONLY thing I did not see was "CREATE TEMPORARY " syntax? If you did this on a TEMP table, > does it generate normal table syntax or TEMPORARY TABLE syntax??? Yeah, the Citus code only handles things that Citus supports in distributed tables. Which is quite a lot, but indeed not everything yet. Temporary and inherited tables are not supported in this code afaik. Possibly more. See the commented out EnsureRelationKindSupported for what should be supported (normal tables and partitioned tables afaik).
Вложения
On Mon, Jun 5, 2023 at 7:43 AM Jelte Fennema <postgres@jeltef.nl> wrote:
On Thu, 1 Jun 2023 at 18:57, Kirk Wolak <wolakk@gmail.com> wrote:
> Can this get turned into a Patch? Were you offering this code up for others (me?) to pull, and work into a patch?
> [If I do the patch, I am not sure it gives you the value of reducing what CITUS has to maintain. But it dawns on
> me that you might be pushing a much bigger patch... But I would take that, as I think there is other value in there]
Attached is a patch which adds the relevant functions from Citus to
Postgres (it compiles without warnings on my machine). I checked with
a few others on the Citus team at Microsoft and everyone thought that
upstreaming this was a good idea, because it's quite a bit of work to
update with every postgres release.
To set expectations though, I don't really have time to work on this
patch. So if you can take it over from here that would be great.
The patch only contains the C functions which generate SQL based on
some oids. The wrappers such as the master_get_table_ddl_events
function were too hard for me to pull out of Citus code, because they
integrated a lot with other pieces. But the bulk of the logic is in
the functions in this patch. Essentially all that
master_get_table_ddl_events does is call the functions in this patch
in the right order.
> The ONLY thing I did not see was "CREATE TEMPORARY " syntax? If you did this on a TEMP table,
> does it generate normal table syntax or TEMPORARY TABLE syntax???
Yeah, the Citus code only handles things that Citus supports in
distributed tables. Which is quite a lot, but indeed not everything
yet. Temporary and inherited tables are not supported in this code
afaik. Possibly more. See the commented out
EnsureRelationKindSupported for what should be supported (normal
tables and partitioned tables afaik).
Jelte,
Thank you for this.
Let me see what I can do with this, it seems like a solid starting point.
At this point, based on previous feedback, finding a way to make get_tabledef() etc. to work as functions is my goal.
I will see how inherited tables and temporary tables will be dealt with.
Hopefully, this transfer works to please anyone concerned with integrating this code into our project from the Citus code.
Kirk...
On Wed, Jun 21, 2023 at 8:52 PM Kirk Wolak <wolakk@gmail.com> wrote:
On Mon, Jun 5, 2023 at 7:43 AM Jelte Fennema <postgres@jeltef.nl> wrote:On Thu, 1 Jun 2023 at 18:57, Kirk Wolak <wolakk@gmail.com> wrote:
> Can this get turned into a Patch? Were you offering this code up for others (me?) to pull, and work into a patch?
> [If I do the patch, I am not sure it gives you the value of reducing what CITUS has to maintain. But it dawns on
> me that you might be pushing a much bigger patch... But I would take that, as I think there is other value in there]
Yeah, the Citus code only handles things that Citus supports in
distributed tables. Which is quite a lot, but indeed not everything
yet. Temporary and inherited tables are not supported in this code
afaik. Possibly more. See the commented out
EnsureRelationKindSupported for what should be supported (normal
tables and partitioned tables afaik).
Okay, apologies for the long delay on this. I have the code Jelte submitted working. And I have (almost) figured out how to add the function so it shows up in the pg_catalog... (I edited files I should not have, I need to know the proper process... Anyone...)
Not sure if it is customary to attach the code when asking about stuff. For the most part, it was what Jelte Gave us with a pg_get_tabledef() wrapper to call...
Here is the output it produces for select pg_get_tabledef('pg_class'::regclass); (Feedback Welcome)
==
My Comments/Questions:
1) I would prefer Legible output, like below
2) I would prefer to leave off COLLATE "C" IFF that is the DB Default
3) The USING heap... I want to pull UNLESS the value is NOT the default (That's a theme in my comments)
4) I *THINK* including the schema would be nice?
5) This version will work with a TEMP table, but NOT EMIT "TEMPORARY"... Thoughts? Is emitting [pg_temp.] good enough?
6) This version enumerates sequence values (Drop always, or Drop if they are the default values?)
7) Should I enable the pg_get_seqdef() code
8) It does NOT handle Inheritance (Yet... Is this important? Is it okay to just give the table structure for this table?)
9) I have not tested against Partitions, etc... I SIMPLY want initial feedback on Formatting
-- Legible:
CREATE TABLE pg_class (oid oid NOT NULL,
relname name NOT NULL COLLATE "C",
relnamespace oid NOT NULL,
reltype oid NOT NULL,
...
reloptions text[] COLLATE "C",
relpartbound pg_node_tree COLLATE "C"
)
-- Too verbose with "DEFAULT" Sequence Values:
CREATE TABLE t1 (id bigint GENERATED BY DEFAULT AS IDENTITY (INCREMENT BY 1 MINVALUE 1 MAXVALUE 9223372036854775807 START WITH 1 CACHE 1 NO CYCLE) NOT NULL,
f1 text
) WITH (autovacuum_vacuum_cost_delay='0', fillfactor='80', autovacuum_vacuum_insert_threshold='-1', autovacuum_analyze_threshold='500000000', autovacuum_vacuum_threshold='500000000', autovacuum_vacuum_scale_factor='1.5')
Thanks,
Kirk...
PS: After I get feedback on Formatting the output, etc. I will gladly generate a new .patch file and send it along. Otherwise Jelte gets 100% of the credit, and I don't want to look like I am changing that.
PS: After I get feedback on Formatting the output, etc. I will gladly generate a new .patch file and send it along. Otherwise Jelte gets 100% of the credit, and I don't want to look like I am changing that.
On Fri, Jun 30, 2023 at 1:56 PM Kirk Wolak <wolakk@gmail.com> wrote:
On Wed, Jun 21, 2023 at 8:52 PM Kirk Wolak <wolakk@gmail.com> wrote:On Mon, Jun 5, 2023 at 7:43 AM Jelte Fennema <postgres@jeltef.nl> wrote:On Thu, 1 Jun 2023 at 18:57, Kirk Wolak <wolakk@gmail.com> wrote:
Definitely have the questions from the previous email, but I CERTAINLY appreciate this output.
(Don't like the +, but pg_get_viewdef() creates the view the same way)...
Will psql doing \st pg_class be able to just call/output this so that the output is nice and clean?
At this point... I will keep pressing forward, cleaning things up. And then send a patch for others to play with....
(Probably bad timing with wrapping up V16)
Will psql doing \st pg_class be able to just call/output this so that the output is nice and clean?
At this point... I will keep pressing forward, cleaning things up. And then send a patch for others to play with....
(Probably bad timing with wrapping up V16)
select pg_get_tabledef('pg_class'::regclass);
pg_get_tabledef
----------------------------------------
CREATE TABLE pg_class (oid oid NOT NULL,+
relname name NOT NULL COLLATE "C", +
relnamespace oid NOT NULL, +
reltype oid NOT NULL, +
reloftype oid NOT NULL, +
relowner oid NOT NULL, +
relam oid NOT NULL, +
relfilenode oid NOT NULL, +
reltablespace oid NOT NULL, +
relpages integer NOT NULL, +
reltuples real NOT NULL, +
relallvisible integer NOT NULL, +
reltoastrelid oid NOT NULL, +
relhasindex boolean NOT NULL, +
relisshared boolean NOT NULL, +
relpersistence "char" NOT NULL, +
relkind "char" NOT NULL, +
relnatts smallint NOT NULL, +
relchecks smallint NOT NULL, +
relhasrules boolean NOT NULL, +
relhastriggers boolean NOT NULL, +
relhassubclass boolean NOT NULL, +
relrowsecurity boolean NOT NULL, +
relforcerowsecurity boolean NOT NULL, +
relispopulated boolean NOT NULL, +
relreplident "char" NOT NULL, +
relispartition boolean NOT NULL, +
relrewrite oid NOT NULL, +
relfrozenxid xid NOT NULL, +
relminmxid xid NOT NULL, +
relacl aclitem[], +
reloptions text[] COLLATE "C", +
relpartbound pg_node_tree COLLATE "C" +
) USING heap