public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v7 1/7] pg_dump: make CLUSTER ON a separate dump object.. 11+ messages / 4 participants [nested] [flat]
* [PATCH v7 1/7] pg_dump: make CLUSTER ON a separate dump object.. @ 2020-11-26 20:37 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Justin Pryzby @ 2020-11-26 20:37 UTC (permalink / raw) ..since it needs to be restored after any child indexes are restored *and attached*. The order needs to be: 1) restore child and parent index (order doesn't matter); 2) attach child index; 3) set cluster on child and parent index (order doesn't matter); --- src/bin/pg_dump/pg_dump.c | 86 ++++++++++++++++++++++++++-------- src/bin/pg_dump/pg_dump.h | 8 ++++ src/bin/pg_dump/pg_dump_sort.c | 8 ++++ 3 files changed, 82 insertions(+), 20 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index d99b61e621..8dc8a42964 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -208,6 +208,7 @@ static void dumpSequence(Archive *fout, TableInfo *tbinfo); static void dumpSequenceData(Archive *fout, TableDataInfo *tdinfo); static void dumpIndex(Archive *fout, IndxInfo *indxinfo); static void dumpIndexAttach(Archive *fout, IndexAttachInfo *attachinfo); +static void dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo); static void dumpStatisticsExt(Archive *fout, StatsExtInfo *statsextinfo); static void dumpConstraint(Archive *fout, ConstraintInfo *coninfo); static void dumpTableConstraintComment(Archive *fout, ConstraintInfo *coninfo); @@ -7092,6 +7093,11 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) i_inddependcollversions; int ntups; + int ncluster = 0; + IndexClusterInfo *clusterinfo; + clusterinfo = (IndexClusterInfo *) + pg_malloc0(numTables * sizeof(IndexClusterInfo)); + for (i = 0; i < numTables; i++) { TableInfo *tbinfo = &tblinfo[i]; @@ -7471,6 +7477,27 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) /* Plain secondary index */ indxinfo[j].indexconstraint = 0; } + + /* Record each table's CLUSTERed index, if any */ + if (indxinfo[j].indisclustered) + { + IndxInfo *index = &indxinfo[j]; + IndexClusterInfo *cluster = &clusterinfo[ncluster]; + + cluster->dobj.objType = DO_INDEX_CLUSTER_ON; + cluster->dobj.catId.tableoid = 0; + cluster->dobj.catId.oid = 0; + AssignDumpId(&cluster->dobj); + cluster->dobj.name = pg_strdup(index->dobj.name); + cluster->dobj.namespace = index->indextable->dobj.namespace; + cluster->index = index; + cluster->indextable = &tblinfo[i]; + + /* The CLUSTER ON depends on its index.. */ + addObjectDependency(&cluster->dobj, index->dobj.dumpId); + + ncluster++; + } } PQclear(res); @@ -10323,6 +10350,9 @@ dumpDumpableObject(Archive *fout, DumpableObject *dobj) case DO_SUBSCRIPTION: dumpSubscription(fout, (SubscriptionInfo *) dobj); break; + case DO_INDEX_CLUSTER_ON: + dumpIndexClusterOn(fout, (IndexClusterInfo *) dobj); + break; case DO_PRE_DATA_BOUNDARY: case DO_POST_DATA_BOUNDARY: /* never dumped, nothing to do */ @@ -16543,6 +16573,41 @@ getAttrName(int attrnum, TableInfo *tblInfo) return NULL; /* keep compiler quiet */ } +/* + * dumpIndexClusterOn + * record that the index is clustered. + */ +static void +dumpIndexClusterOn(Archive *fout, IndexClusterInfo *clusterinfo) +{ + DumpOptions *dopt = fout->dopt; + TableInfo *tbinfo = clusterinfo->indextable; + char *qindxname; + PQExpBuffer q; + + if (dopt->dataOnly) + return; + + q = createPQExpBuffer(); + qindxname = pg_strdup(fmtId(clusterinfo->dobj.name)); + + /* index name is not qualified in this syntax */ + appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER ON %s;\n", + fmtQualifiedDumpable(tbinfo), qindxname); + + if (clusterinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) + ArchiveEntry(fout, clusterinfo->dobj.catId, clusterinfo->dobj.dumpId, + ARCHIVE_OPTS(.tag = clusterinfo->dobj.name, + .namespace = tbinfo->dobj.namespace->dobj.name, + .owner = tbinfo->rolname, + .description = "INDEX CLUSTER ON", + .section = SECTION_POST_DATA, + .createStmt = q->data)); + + destroyPQExpBuffer(q); + free(qindxname); +} + /* * dumpIndex * write out to fout a user-defined index @@ -16597,16 +16662,6 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo) * similar code in dumpConstraint! */ - /* If the index is clustered, we need to record that. */ - if (indxinfo->indisclustered) - { - appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER", - fmtQualifiedDumpable(tbinfo)); - /* index name is not qualified in this syntax */ - appendPQExpBuffer(q, " ON %s;\n", - qindxname); - } - /* * If the index has any statistics on some of its columns, generate * the associated ALTER INDEX queries. @@ -16933,16 +16988,6 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo) * similar code in dumpIndex! */ - /* If the index is clustered, we need to record that. */ - if (indxinfo->indisclustered) - { - appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER", - fmtQualifiedDumpable(tbinfo)); - /* index name is not qualified in this syntax */ - appendPQExpBuffer(q, " ON %s;\n", - fmtId(indxinfo->dobj.name)); - } - /* If the index defines identity, we need to record that. */ if (indxinfo->indisreplident) { @@ -18448,6 +18493,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs, break; case DO_INDEX: case DO_INDEX_ATTACH: + case DO_INDEX_CLUSTER_ON: case DO_STATSEXT: case DO_REFRESH_MATVIEW: case DO_TRIGGER: diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 1290f9659b..57def4c009 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -54,6 +54,7 @@ typedef enum DO_ATTRDEF, DO_INDEX, DO_INDEX_ATTACH, + DO_INDEX_CLUSTER_ON, DO_STATSEXT, DO_RULE, DO_TRIGGER, @@ -386,6 +387,13 @@ typedef struct _indxInfo DumpId indexconstraint; } IndxInfo; +typedef struct _indexClusterInfo +{ + DumpableObject dobj; + TableInfo *indextable; /* link to table the index is for */ + IndxInfo *index; /* link to index itself */ +} IndexClusterInfo; + typedef struct _indexAttachInfo { DumpableObject dobj; diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 46461fb6a1..dd5b233196 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -75,6 +75,7 @@ enum dbObjectTypePriorities PRIO_CONSTRAINT, PRIO_INDEX, PRIO_INDEX_ATTACH, + PRIO_INDEX_CLUSTER_ON, PRIO_STATSEXT, PRIO_RULE, PRIO_TRIGGER, @@ -108,6 +109,7 @@ static const int dbObjectTypePriority[] = PRIO_ATTRDEF, /* DO_ATTRDEF */ PRIO_INDEX, /* DO_INDEX */ PRIO_INDEX_ATTACH, /* DO_INDEX_ATTACH */ + PRIO_INDEX_CLUSTER_ON, /* DO_INDEX_CLUSTER_ON */ PRIO_STATSEXT, /* DO_STATSEXT */ PRIO_RULE, /* DO_RULE */ PRIO_TRIGGER, /* DO_TRIGGER */ @@ -136,6 +138,7 @@ static const int dbObjectTypePriority[] = PRIO_PUBLICATION, /* DO_PUBLICATION */ PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */ PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */ + }; StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1), @@ -1348,6 +1351,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize) "INDEX ATTACH %s (ID %d)", obj->name, obj->dumpId); return; + case DO_INDEX_CLUSTER_ON: + snprintf(buf, bufsize, + "INDEX CLUSTER ON %s (ID %d)", + obj->name, obj->dumpId); + return; case DO_STATSEXT: snprintf(buf, bufsize, "STATISTICS %s (ID %d OID %u)", -- 2.17.0 --AA9g+nFNFPYNJKiL Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0002-Implement-CLUSTER-of-partitioned-table.patch" ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-05 09:15 Amit Kapila <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Kapila @ 2022-01-05 09:15 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: Peter Smith <[email protected]>; Euler Taveira <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Dec 28, 2021 at 6:33 PM [email protected] <[email protected]> wrote: > > On Mon, Dec 27, 2021 9:19 PM Hou Zhijie <[email protected]> wrote: > > On Mon, Dec 27, 2021 9:16 PM [email protected] <[email protected]> > > wrote: > > > On Thur, Dec 23, 2021 4:28 PM Peter Smith <[email protected]> wrote: > > > > Here is the v54* patch set: > > > > > > Attach the v55 patch set which add the following testcases in 0002 patch. > > When reviewing the row filter patch, I found few things that could be improved. > 1) We could transform the same row filter expression twice when > ALTER PUBLICATION ... SET TABLE WHERE (...). Because we invoke > GetTransformedWhereClause in both AlterPublicationTables() and > publication_add_relation(). I was thinking it might be better if we only > transform the expression once in AlterPublicationTables(). > > 2) When transforming the expression, we didn’t set the correct p_sourcetext. > Since we need to transforming serval expressions which belong to different > relations, I think it might be better to pass queryString down to the actual > transform function and set p_sourcetext to the actual queryString. > I have tried the following few examples to check the error_position and it seems to be showing correct position without your 0004 patch. postgres=# create publication pub for table t1 where (10); ERROR: argument of PUBLICATION WHERE must be type boolean, not type integer LINE 1: create publication pub for table t1 where (10); ^ Also, transformPubWhereClauses() seems to be returning the same list as it was passed to it. Do we really need to return anything from transformPubWhereClauses()? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-05 10:51 Amit Kapila <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Kapila @ 2022-01-05 10:51 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: Peter Smith <[email protected]>; Euler Taveira <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Jan 5, 2022 at 2:45 PM Amit Kapila <[email protected]> wrote: > > On Tue, Dec 28, 2021 at 6:33 PM [email protected] > <[email protected]> wrote: > > > > On Mon, Dec 27, 2021 9:19 PM Hou Zhijie <[email protected]> wrote: > > > On Mon, Dec 27, 2021 9:16 PM [email protected] <[email protected]> > > > wrote: > > > > On Thur, Dec 23, 2021 4:28 PM Peter Smith <[email protected]> wrote: > > > > > Here is the v54* patch set: > > > > > > > > Attach the v55 patch set which add the following testcases in 0002 patch. > > > > When reviewing the row filter patch, I found few things that could be improved. > > 1) We could transform the same row filter expression twice when > > ALTER PUBLICATION ... SET TABLE WHERE (...). Because we invoke > > GetTransformedWhereClause in both AlterPublicationTables() and > > publication_add_relation(). I was thinking it might be better if we only > > transform the expression once in AlterPublicationTables(). > > > > 2) When transforming the expression, we didn’t set the correct p_sourcetext. > > Since we need to transforming serval expressions which belong to different > > relations, I think it might be better to pass queryString down to the actual > > transform function and set p_sourcetext to the actual queryString. > > > > I have tried the following few examples to check the error_position > and it seems to be showing correct position without your 0004 patch. > postgres=# create publication pub for table t1 where (10); > ERROR: argument of PUBLICATION WHERE must be type boolean, not type integer > LINE 1: create publication pub for table t1 where (10); > > ^ > I understand why the error position could vary even though it is showing the correct location in the above example after reading another related email [1]. > Also, transformPubWhereClauses() seems to be returning the same list > as it was passed to it. Do we really need to return anything from > transformPubWhereClauses()? > One more point about this function: the patch seems to be doing some work even when where clause is not specified which can be avoided. Another minor comment: +static bool pgoutput_row_filter(enum ReorderBufferChangeType changetype, Do we need to specify the 'enum' type before changetype parameter? [1] - https://www.postgresql.org/message-id/1513381.1640626456%40sss.pgh.pa.us -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-06 03:12 Peter Smith <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Peter Smith @ 2022-01-06 03:12 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: [email protected] <[email protected]>; Euler Taveira <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Jan 5, 2022 at 9:52 PM Amit Kapila <[email protected]> wrote: > ... > Another minor comment: > +static bool pgoutput_row_filter(enum ReorderBufferChangeType changetype, > > Do we need to specify the 'enum' type before changetype parameter? > That is because there is currently no typedef for the enum ReorderBufferChangeType. Of course, it is easy to add a typedef and then this 'enum' is not needed in the signature, but I wasn't sure if adding a new typedef strictly belonged as part of this Row-Filter patch or not. ------ Kind Regards. Peter Smith. Fujitsu Australia. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-06 04:18 Amit Kapila <[email protected]> parent: Peter Smith <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Kapila @ 2022-01-06 04:18 UTC (permalink / raw) To: Peter Smith <[email protected]>; +Cc: [email protected] <[email protected]>; Euler Taveira <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Jan 6, 2022 at 8:43 AM Peter Smith <[email protected]> wrote: > > On Wed, Jan 5, 2022 at 9:52 PM Amit Kapila <[email protected]> wrote: > > > ... > > > Another minor comment: > > +static bool pgoutput_row_filter(enum ReorderBufferChangeType changetype, > > > > Do we need to specify the 'enum' type before changetype parameter? > > > > That is because there is currently no typedef for the enum > ReorderBufferChangeType. > But I see that the 0002 patch is already adding the required typedef. > Of course, it is easy to add a typedef and then this 'enum' is not > needed in the signature, but I wasn't sure if adding a new typedef > strictly belonged as part of this Row-Filter patch or not. > I don't see any harm in doing so. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-06 13:11 Euler Taveira <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Euler Taveira @ 2022-01-06 13:11 UTC (permalink / raw) To: Amit Kapila <[email protected]>; Peter Smith <[email protected]>; +Cc: [email protected] <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Jan 6, 2022, at 1:18 AM, Amit Kapila wrote: > On Thu, Jan 6, 2022 at 8:43 AM Peter Smith <[email protected]> wrote: > > > > On Wed, Jan 5, 2022 at 9:52 PM Amit Kapila <[email protected]> wrote: > > > > > ... > > > > > Another minor comment: > > > +static bool pgoutput_row_filter(enum ReorderBufferChangeType changetype, > > > > > > Do we need to specify the 'enum' type before changetype parameter? > > > > > > > That is because there is currently no typedef for the enum > > ReorderBufferChangeType. > > > > But I see that the 0002 patch is already adding the required typedef. IMO we shouldn't reuse ReorderBufferChangeType. For a long-term solution, it is fragile. ReorderBufferChangeType has values that do not matter for row filter and it relies on the fact that REORDER_BUFFER_CHANGE_INSERT, REORDER_BUFFER_CHANGE_UPDATE and REORDER_BUFFER_CHANGE_DELETE are the first 3 values from the enum, otherwise, it breaks rfnodes and no_filters in pgoutput_row_filter(). I suggest a separate enum that contains only these 3 values. enum RowFilterPublishAction { PUBLISH_ACTION_INSERT, PUBLISH_ACTION_UPDATE, PUBLISH_ACTION_DELETE }; -- Euler Taveira EDB https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-07 04:14 Amit Kapila <[email protected]> parent: Euler Taveira <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Kapila @ 2022-01-07 04:14 UTC (permalink / raw) To: Euler Taveira <[email protected]>; +Cc: Peter Smith <[email protected]>; [email protected] <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Jan 6, 2022 at 6:42 PM Euler Taveira <[email protected]> wrote: > > On Thu, Jan 6, 2022, at 1:18 AM, Amit Kapila wrote: > > On Thu, Jan 6, 2022 at 8:43 AM Peter Smith <[email protected]> wrote: > > > > On Wed, Jan 5, 2022 at 9:52 PM Amit Kapila <[email protected]> wrote: > > > > > ... > > > > > Another minor comment: > > > +static bool pgoutput_row_filter(enum ReorderBufferChangeType changetype, > > > > > > Do we need to specify the 'enum' type before changetype parameter? > > > > > > > That is because there is currently no typedef for the enum > > ReorderBufferChangeType. > > > > But I see that the 0002 patch is already adding the required typedef. > > IMO we shouldn't reuse ReorderBufferChangeType. For a long-term solution, it is > fragile. ReorderBufferChangeType has values that do not matter for row filter > and it relies on the fact that REORDER_BUFFER_CHANGE_INSERT, > REORDER_BUFFER_CHANGE_UPDATE and REORDER_BUFFER_CHANGE_DELETE are the first 3 > values from the enum, otherwise, it breaks rfnodes and no_filters in > pgoutput_row_filter(). > I think you mean to say it will break in pgoutput_row_filter_init(). I see your point but OTOH, if we do what you are suggesting then don't we need an additional mapping between ReorderBufferChangeType and RowFilterPublishAction as row filter and pgoutput_change API need to use those values. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-07 06:35 Amit Kapila <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Kapila @ 2022-01-07 06:35 UTC (permalink / raw) To: Euler Taveira <[email protected]>; +Cc: Peter Smith <[email protected]>; [email protected] <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Jan 7, 2022 at 9:44 AM Amit Kapila <[email protected]> wrote: > > On Thu, Jan 6, 2022 at 6:42 PM Euler Taveira <[email protected]> wrote: > > > > IMO we shouldn't reuse ReorderBufferChangeType. For a long-term solution, it is > > fragile. ReorderBufferChangeType has values that do not matter for row filter > > and it relies on the fact that REORDER_BUFFER_CHANGE_INSERT, > > REORDER_BUFFER_CHANGE_UPDATE and REORDER_BUFFER_CHANGE_DELETE are the first 3 > > values from the enum, otherwise, it breaks rfnodes and no_filters in > > pgoutput_row_filter(). > > > > I think you mean to say it will break in pgoutput_row_filter_init(). I > see your point but OTOH, if we do what you are suggesting then don't > we need an additional mapping between ReorderBufferChangeType and > RowFilterPublishAction as row filter and pgoutput_change API need to > use those values. > Can't we use 0,1,2 as indexes for rfnodes/no_filters based on change type as they are local variables as that will avoid the fragileness you are worried about. I am slightly hesitant to introduce new enum when we are already using reorder buffer change type in pgoutput.c. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-07 09:05 Amit Kapila <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Amit Kapila @ 2022-01-07 09:05 UTC (permalink / raw) To: Euler Taveira <[email protected]>; +Cc: Peter Smith <[email protected]>; [email protected] <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Jan 7, 2022 at 12:05 PM Amit Kapila <[email protected]> wrote: > > On Fri, Jan 7, 2022 at 9:44 AM Amit Kapila <[email protected]> wrote: > > > > On Thu, Jan 6, 2022 at 6:42 PM Euler Taveira <[email protected]> wrote: > > > > > > IMO we shouldn't reuse ReorderBufferChangeType. For a long-term solution, it is > > > fragile. ReorderBufferChangeType has values that do not matter for row filter > > > and it relies on the fact that REORDER_BUFFER_CHANGE_INSERT, > > > REORDER_BUFFER_CHANGE_UPDATE and REORDER_BUFFER_CHANGE_DELETE are the first 3 > > > values from the enum, otherwise, it breaks rfnodes and no_filters in > > > pgoutput_row_filter(). > > > > > > > I think you mean to say it will break in pgoutput_row_filter_init(). I > > see your point but OTOH, if we do what you are suggesting then don't > > we need an additional mapping between ReorderBufferChangeType and > > RowFilterPublishAction as row filter and pgoutput_change API need to > > use those values. > > > > Can't we use 0,1,2 as indexes for rfnodes/no_filters based on change > type as they are local variables as that will avoid the fragileness > you are worried about. I am slightly hesitant to introduce new enum > when we are already using reorder buffer change type in pgoutput.c. > Euler, I have one more question about this patch for you. I see that in the patch we are calling coerce_to_target_type() in pgoutput_row_filter_init_expr() but do we really need the same? We already do that via transformPubWhereClauses->transformWhereClause->coerce_to_boolean before storing where clause expression. It is not clear to me why that is required? We might want to add a comment if that is required. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-07 17:50 Euler Taveira <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Euler Taveira @ 2022-01-07 17:50 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Peter Smith <[email protected]>; [email protected] <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Jan 7, 2022, at 6:05 AM, Amit Kapila wrote: > Euler, I have one more question about this patch for you. I see that > in the patch we are calling coerce_to_target_type() in > pgoutput_row_filter_init_expr() but do we really need the same? We > already do that via > transformPubWhereClauses->transformWhereClause->coerce_to_boolean > before storing where clause expression. It is not clear to me why that > is required? We might want to add a comment if that is required. It is redundant. It seems an additional safeguard that we should be removed. Good catch. -- Euler Taveira EDB https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: row filtering for logical replication @ 2022-01-08 04:47 Amit Kapila <[email protected]> parent: Euler Taveira <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Amit Kapila @ 2022-01-08 04:47 UTC (permalink / raw) To: Euler Taveira <[email protected]>; +Cc: Peter Smith <[email protected]>; [email protected] <[email protected]>; Greg Nancarrow <[email protected]>; vignesh C <[email protected]>; Ajin Cherian <[email protected]>; [email protected] <[email protected]>; Dilip Kumar <[email protected]>; Rahila Syed <[email protected]>; Peter Eisentraut <[email protected]>; Önder Kalacı <[email protected]>; japin <[email protected]>; Michael Paquier <[email protected]>; David Steele <[email protected]>; Craig Ringer <[email protected]>; Amit Langote <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Jan 7, 2022 at 11:20 PM Euler Taveira <[email protected]> wrote: > > On Fri, Jan 7, 2022, at 6:05 AM, Amit Kapila wrote: > > Euler, I have one more question about this patch for you. I see that > in the patch we are calling coerce_to_target_type() in > pgoutput_row_filter_init_expr() but do we really need the same? We > already do that via > transformPubWhereClauses->transformWhereClause->coerce_to_boolean > before storing where clause expression. It is not clear to me why that > is required? We might want to add a comment if that is required. > > It is redundant. It seems an additional safeguard that we should be removed. > Good catch. > Thanks for the confirmation. Actually, it was raised by Vignesh in his email [1]. [1] - https://www.postgresql.org/message-id/CALDaNm1_JVg_hqoGex_FVca_HPF46n9oDDB9dsp1SrPuaVpp-w%40mail.gma... -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2022-01-08 04:47 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-26 20:37 [PATCH v7 1/7] pg_dump: make CLUSTER ON a separate dump object.. Justin Pryzby <[email protected]> 2022-01-05 09:15 Re: row filtering for logical replication Amit Kapila <[email protected]> 2022-01-05 10:51 ` Re: row filtering for logical replication Amit Kapila <[email protected]> 2022-01-06 03:12 ` Re: row filtering for logical replication Peter Smith <[email protected]> 2022-01-06 04:18 ` Re: row filtering for logical replication Amit Kapila <[email protected]> 2022-01-06 13:11 ` Re: row filtering for logical replication Euler Taveira <[email protected]> 2022-01-07 04:14 ` Re: row filtering for logical replication Amit Kapila <[email protected]> 2022-01-07 06:35 ` Re: row filtering for logical replication Amit Kapila <[email protected]> 2022-01-07 09:05 ` Re: row filtering for logical replication Amit Kapila <[email protected]> 2022-01-07 17:50 ` Re: row filtering for logical replication Euler Taveira <[email protected]> 2022-01-08 04:47 ` Re: row filtering for logical replication Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox