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]>
2022-01-05 10:51 ` Re: row filtering for logical replication 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 09:15 Re: row filtering for logical replication Amit Kapila <[email protected]>
@ 2022-01-05 10:51 ` Amit Kapila <[email protected]>
2022-01-06 03:12 ` Re: row filtering for logical replication Peter Smith <[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-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 ` Peter Smith <[email protected]>
2022-01-06 04:18 ` Re: row filtering for logical replication 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-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 ` Amit Kapila <[email protected]>
2022-01-06 13:11 ` Re: row filtering for logical replication Euler Taveira <[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-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 ` Euler Taveira <[email protected]>
2022-01-07 04:14 ` Re: row filtering for logical replication 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-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 ` Amit Kapila <[email protected]>
2022-01-07 06:35 ` Re: row filtering for logical replication Amit Kapila <[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-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 ` Amit Kapila <[email protected]>
2022-01-07 09:05 ` Re: row filtering for logical replication 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-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 ` Amit Kapila <[email protected]>
2022-01-07 17:50 ` Re: row filtering for logical replication Euler Taveira <[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-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 ` Euler Taveira <[email protected]>
2022-01-08 04:47 ` Re: row filtering for logical replication 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-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 ` Amit Kapila <[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