Re: [HACKERS] sequences and pg_upgrade
On 11/2/16 2:34 AM, Michael Paquier wrote: > I had a look at those fresh patches, and 0001 looks like a good thing. > This makes the separation between sequences and table data dump > cleaner. I ran some tests with pg_upgrade and 0002, and things are > clear. And +1 for the way done in the patch, aka no options of pg_dump > exposed to user, still keep the option tracking as a separate value. Committed, thanks. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
On Mon, Oct 31, 2016 at 9:53 PM, Peter Eisentraut
wrote:
> On 9/30/16 12:50 PM, Anastasia Lubennikova wrote:
>> The patches are good, no complaints.
>> But again, I have the same question.
>> I was confused, why do we always dump sequence data,
>> because I'd overlooked the --sequence-data key. I'd rather leave this
>> option,
>> because it's quite non intuitive behaviour...
>> /* dump sequence data even in schema-only mode */
>
> Here are rebased patches.
>
> Regarding your question: The initial patch had a separate option for
> this behavior, which was then used by pg_upgrade. It was commented that
> this option is not useful outside of pg_upgrade, so it doesn't need to
> be exposed as a user-facing option. I agreed with that and removed the
> option. We can always add the option back easily if someone really
> wants it, but so far no use case has been presented. So I suggest we
> proceed with this proposal ignoring whether this option is exposed or not.
I had a look at those fresh patches, and 0001 looks like a good thing.
This makes the separation between sequences and table data dump
cleaner. I ran some tests with pg_upgrade and 0002, and things are
clear. And +1 for the way done in the patch, aka no options of pg_dump
exposed to user, still keep the option tracking as a separate value.
One small thing here:
static void
-getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids)
+getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables,
bool oids, char relkind)
{
int i;
for (i = 0; i < numTables; i++)
{
- if (tblinfo[i].dobj.dump & DUMP_COMPONENT_DATA)
+ if (tblinfo[i].dobj.dump & DUMP_COMPONENT_DATA &&
+ (!relkind || tblinfo[i].relkind == relkind))
makeTableDataInfo(dopt, &(tblinfo[i]), oids)
One idea here would be to have an extra routine, getSequenceData and
not extend getTableData() with relkind as extra argument. I am fine
with the way patch does things, so I just switched the patch as ready
for committer.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
On 9/30/16 12:50 PM, Anastasia Lubennikova wrote: > The patches are good, no complaints. > But again, I have the same question. > I was confused, why do we always dump sequence data, > because I'd overlooked the --sequence-data key. I'd rather leave this > option, > because it's quite non intuitive behaviour... > /* dump sequence data even in schema-only mode */ Here are rebased patches. Regarding your question: The initial patch had a separate option for this behavior, which was then used by pg_upgrade. It was commented that this option is not useful outside of pg_upgrade, so it doesn't need to be exposed as a user-facing option. I agreed with that and removed the option. We can always add the option back easily if someone really wants it, but so far no use case has been presented. So I suggest we proceed with this proposal ignoring whether this option is exposed or not. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >From d2b98ba5df815018dac1650134398c1bac7164a4 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Tue, 23 Aug 2016 12:00:00 -0400 Subject: [PATCH v3 1/2] pg_dump: Separate table and sequence data object types Instead of handling both sequence data and table data internally as "table data", handle sequences separately under a "sequence set" type. We already handled materialized view data differently, so it makes the code somewhat cleaner to handle each relation kind separately at the top level. This does not change the output format, since there already was a separate "SEQUENCE SET" archive entry type. A noticeable difference is that SEQUENCE SET entries now always appear after TABLE DATA entries. And in parallel mode there is less sorting to do, because the sequence data entries are no longer considered table data. --- src/bin/pg_dump/pg_dump.c | 11 +++ src/bin/pg_dump/pg_dump.h | 1 + src/bin/pg_dump/pg_dump_sort.c | 28 +--- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 4da297f..3485cab 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -2090,6 +2090,8 @@ makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids) if (tbinfo->relkind == RELKIND_MATVIEW) tdinfo->dobj.objType = DO_REFRESH_MATVIEW; + else if (tbinfo->relkind == RELKIND_SEQUENCE) + tdinfo->dobj.objType = DO_SEQUENCE_SET; else tdinfo->dobj.objType = DO_TABLE_DATA; @@ -8498,11 +8500,11 @@ dumpDumpableObject(Archive *fout, DumpableObject *dobj) case DO_TRANSFORM: dumpTransform(fout, (TransformInfo *) dobj); break; + case DO_SEQUENCE_SET: + dumpSequenceData(fout, (TableDataInfo *) dobj); + break; case DO_TABLE_DATA: - if (((TableDataInfo *) dobj)->tdtable->relkind == RELKIND_SEQUENCE) -dumpSequenceData(fout, (TableDataInfo *) dobj); - else -dumpTableData(fout, (TableDataInfo *) dobj); + dumpTableData(fout, (TableDataInfo *) dobj); break; case DO_DUMMY_TYPE: /* table rowtypes and array types are never dumped separately */ @@ -16226,6 +16228,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs, addObjectDependency(preDataBound, dobj->dumpId); break; case DO_TABLE_DATA: + case DO_SEQUENCE_SET: case DO_BLOB_DATA: /* Data objects: must come between the boundaries */ addObjectDependency(dobj, preDataBound->dumpId); diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index a60cf95..642c4d5 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -63,6 +63,7 @@ typedef enum DO_PROCLANG, DO_CAST, DO_TABLE_DATA, + DO_SEQUENCE_SET, DO_DUMMY_TYPE, DO_TSPARSER, DO_TSDICT, diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 195b84a..5b96334 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -47,14 +47,15 @@ static const int dbObjectTypePriority[] = 11, /* DO_CONVERSION */ 18, /* DO_TABLE */ 20, /* DO_ATTRDEF */ - 27, /* DO_INDEX */ - 28, /* DO_RULE */ - 29, /* DO_TRIGGER */ - 26, /* DO_CONSTRAINT */ - 30, /* DO_FK_CONSTRAINT */ + 28, /* DO_INDEX */ + 29, /* DO_RULE */ + 30, /* DO_TRIGGER */ + 27, /* DO_CONSTRAINT */ + 31, /* DO_FK_CONSTRAINT */ 2, /* DO_PROCLANG */ 10, /* DO_CAST */ 23, /* DO_TABLE_DATA */ + 24, /* DO_SEQUENCE_SET */ 19, /* DO_DUMMY_TYPE */ 12, /* DO_TSPARSER */ 14, /* DO_TSDICT */ @@ -62,15 +63,15 @@ static const int dbObjectTypePriority[] = 15, /* DO_TSCONFIG */ 16, /* DO_FDW */ 17, /* DO_FOREIGN_SERVER */ - 31, /* DO_DEFAULT_ACL */ + 32, /* DO_DEFAULT_ACL */ 3, /* DO_TRANSFORM */ 21, /* DO_BLOB */ - 24, /* DO_BLOB_DATA */ + 25, /* DO_BLOB_DATA */ 22, /* DO_PRE_DA
Re: [HACKERS] sequences and pg_upgrade
On Sat, Oct 1, 2016 at 1:50 AM, Anastasia Lubennikova wrote: > 23.09.2016 21:06, Peter Eisentraut: >> >> Here is an updated patch set. Compared to the initial set, I have >> changed pg_dump's sorting priorities so that sequence data is always >> after table data. This would otherwise have introduced a problem >> because sortDataAndIndexObjectsBySize() only considers consecutive >> DO_TABLE_DATA entries. Also, I have removed the separate >> --sequence-data switch from pg_dump and made it implicit in >> --binary-upgrade. (So the previous patches 0002 and 0003 have been >> combined, because it's no longer a separate feature.) >> > > The patches are good, no complaints. > But again, I have the same question. > I was confused, why do we always dump sequence data, > because I'd overlooked the --sequence-data key. I'd rather leave this > option, > because it's quite non intuitive behaviour... > /* dump sequence data even in schema-only mode */ Moved to next CF. This is fresh. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
23.09.2016 21:06, Peter Eisentraut: Here is an updated patch set. Compared to the initial set, I have changed pg_dump's sorting priorities so that sequence data is always after table data. This would otherwise have introduced a problem because sortDataAndIndexObjectsBySize() only considers consecutive DO_TABLE_DATA entries. Also, I have removed the separate --sequence-data switch from pg_dump and made it implicit in --binary-upgrade. (So the previous patches 0002 and 0003 have been combined, because it's no longer a separate feature.) The patches are good, no complaints. But again, I have the same question. I was confused, why do we always dump sequence data, because I'd overlooked the --sequence-data key. I'd rather leave this option, because it's quite non intuitive behaviour... /* dump sequence data even in schema-only mode */ -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
Here is an updated patch set. Compared to the initial set, I have changed pg_dump's sorting priorities so that sequence data is always after table data. This would otherwise have introduced a problem because sortDataAndIndexObjectsBySize() only considers consecutive DO_TABLE_DATA entries. Also, I have removed the separate --sequence-data switch from pg_dump and made it implicit in --binary-upgrade. (So the previous patches 0002 and 0003 have been combined, because it's no longer a separate feature.) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services v2-0001-pg_dump-Separate-table-and-sequence-data-object-t.patch Description: invalid/octet-stream v2-0002-pg_upgrade-Upgrade-sequence-data-via-pg_dump.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
15.09.2016 15:29, Peter Eisentraut: On 9/14/16 8:52 AM, Anastasia Lubennikova wrote: Could you clarify, please, why do we dump sequence in schemaOnly mode? + if (dopt.schemaOnly && dopt.sequence_data) + getSequenceData(&dopt, tblinfo, numTables, dopt.oids); The point of this patch is that with the new option, you can dump sequence data (but not table data) alongside with the schema. This would be used by pg_upgrade for the reasons described at the beginning of the thread. Oh, thank you. Now I see. Somewhy I thought that it *always* dumps sequence data in schemaOnly mode. Which is definitely not true. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
On 9/14/16 8:52 AM, Anastasia Lubennikova wrote: > Could you clarify, please, why do we dump sequence in schemaOnly mode? > + if (dopt.schemaOnly && dopt.sequence_data) > + getSequenceData(&dopt, tblinfo, numTables, dopt.oids); The point of this patch is that with the new option, you can dump sequence data (but not table data) alongside with the schema. This would be used by pg_upgrade for the reasons described at the beginning of the thread. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation:tested, failed
Thank you for the patch.
As I see there are no objections in the discussion, all the patches look clear.
Could you clarify, please, why do we dump sequence in schemaOnly mode?
+ if (dopt.schemaOnly && dopt.sequence_data)
+ getSequenceData(&dopt, tblinfo, numTables, dopt.oids);
Example:
postgres=# create table t(i serial, data text);
postgres=# insert into t(data) values ('aaa');
pg_dump -d postgres --sequence-data --schema-only > ../reviews/dump_pg
Then restore it into newdb and add new value.
newdb=# insert into t(data) values ('aaa');
INSERT 0 1
newdb=# select * from t;
i | data
---+--
2 | aaa
I'm not an experienced user, but I thought that while doing dump/restore
of schema of database we reset all the data. Why should the table in newly
created (via pg_restore) database have non-default sequence value?
I also did some other tests and all of them were passed.
One more thing to do is a documentation for the new option.
You should update help() function in pg_dump.c and also add some
notes to pg_dump.sgml and probably to pgupgrade.sgml.
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
On 2016-08-30 08:46:48 -0400, Peter Eisentraut wrote: > I was toying with a couple of ideas that would involve changing the > storage of sequences. (Say, for the sake of discussion, removing the > problematic/useless sequence_name field.) I'd be quite interested to know what changes that are... > I think the other solution mentioned in that thread would also work: > Have pg_upgrade treat sequences more like system catalogs, whose format > changes between major releases, and transferred them via the > dump/restore route. So instead of copying the disk files, issue a > setval call, and the sequence should be all set up. +1. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
On Tue, Aug 30, 2016 at 08:46:48AM -0400, Peter Eisentraut wrote: > I think the other solution mentioned in that thread would also work: > Have pg_upgrade treat sequences more like system catalogs, whose format > changes between major releases, and transferred them via the > dump/restore route. So instead of copying the disk files, issue a > setval call, and the sequence should be all set up. > > Am I missing anything? Looks straight-forward to me. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sequences and pg_upgrade
Peter Eisentraut writes: > I was toying with a couple of ideas that would involve changing the > storage of sequences. (Say, for the sake of discussion, removing the > problematic/useless sequence_name field.) This would cause problems for > pg_upgrade, because pg_upgrade copies the "heap" storage of sequences > like it does for normal tables, and we have no facilities for effecting > any changes during that. > There was a previous discussion in the early days of pg_migrator, which > resulted in the current behavior: > https://www.postgresql.org/message-id/flat/20090713220112.GF7933%40klana.box > This also alluded to what I think was the last change in the sequence > storage format (10a3471bed7b57fb986a5be8afdee5f0dda419de) between > versions 8.3 and 8.4. How did pg_upgrade handle that? I think it probably never did handle that. pg_upgrade doesn't currently claim to support migrating from 8.3, and the thread you mention shows that the original attempt at 8.3->8.4 migration crashed-and-burned for numerous unrelated reasons. We may not have ever got to the point of noticing that 10a3471be also created a problem. > I think the other solution mentioned in that thread would also work: > Have pg_upgrade treat sequences more like system catalogs, whose format > changes between major releases, and transferred them via the > dump/restore route. So instead of copying the disk files, issue a > setval call, and the sequence should be all set up. Seems reasonable. If you're proposing to expose --sequence-data as a user-visible option, the patch set lacks documentation. But I wonder whether it shouldn't simply be a side-effect of --binary-upgrade. It seems a tad non-orthogonal for a user switch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
