On Tue, Nov 30, 2021 at 12:33 PM Ajin Cherian <itsa...@gmail.com> wrote:
>
> On Thu, Nov 25, 2021 at 2:22 PM Peter Smith <smithpb2...@gmail.com> wrote:
> >
> > Thanks for all the review comments so far! We are endeavouring to keep
> > pace with them.
> >
> > All feedback is being tracked and we will fix and/or reply to everything
> > ASAP.
> >
> > Meanwhile, PSA the latest set of v42* patches.
> >
> > This version was mostly a patch restructuring exercise but it also
> > addresses some minor review comments in passing.
> >
>
> Addressed more review comments, in the attached patch-set v43. 5
> patches carried forward from v42.
> This patch-set contains the following fixes:
>
> On Tue, Nov 23, 2021 at 1:28 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
> >
> > in pgoutput_row_filter, we are dropping the slots if there are some
> > old slots in the RelationSyncEntry. But then I noticed that in
> > rel_sync_cache_relation_cb(), also we are doing that but only for the
> > scantuple slot. So IMHO, rel_sync_cache_relation_cb(), is only place
> > setting entry->rowfilter_valid to false; so why not drop all the slot
> > that time only and in pgoutput_row_filter(), you can just put an
> > assert?
> >
>
> Moved all the dropping of slots to rel_sync_cache_relation_cb()
>
> > +static bool
> > +pgoutput_row_filter_virtual(Relation relation, TupleTableSlot *slot,
> > RelationSyncEntry *entry)
> > +{
> > + EState *estate;
> > + ExprContext *ecxt;
> >
> >
> > pgoutput_row_filter_virtual and pgoutput_row_filter are exactly same
> > except, ExecStoreHeapTuple(), so why not just put one check based on
> > whether a slot is passed or not, instead of making complete duplicate
> > copy of the function.
>
> Removed pgoutput_row_filter_virtual
>
> > oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> > tupdesc = CreateTupleDescCopy(tupdesc);
> > entry->scantuple = MakeSingleTupleTableSlot(tupdesc,
> > &TTSOpsHeapTuple);
> >
> > Why do we need to copy the tupledesc? do we think that we need to have
> > this slot even if we close the relation, if so can you add the
> > comments explaining why we are making a copy here.
>
> This code has been modified, and comments added.
>
> On Tue, Nov 23, 2021 at 8:02 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> > One more thing related to this code:
> > pgoutput_row_filter()
> > {
> > ..
> > + if (!entry->rowfilter_valid)
> > {
> > ..
> > + oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> > + tupdesc = CreateTupleDescCopy(tupdesc);
> > + entry->scantuple = MakeSingleTupleTableSlot(tupdesc, &TTSOpsHeapTuple);
> > + MemoryContextSwitchTo(oldctx);
> > ..
> > }
> >
> > Why do we need to initialize scantuple here unless we are sure that
> > the row filter is going to get associated with this relentry? I think
> > when there is no row filter then this allocation is not required.
> >
>
> Modified as suggested.
>
> On Tue, Nov 23, 2021 at 10:52 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > In 0003 patch, why is below change required?
> > --- a/src/backend/replication/pgoutput/pgoutput.c
> > +++ b/src/backend/replication/pgoutput/pgoutput.c
> > @@ -1,4 +1,4 @@
> > -/*-------------------------------------------------------------------------
> > +/*------------------------------------------------------------------------
> > *
> > * pgoutput.c
> >
>
> Removed.
>
> >
> > After above, rearrange the code in pgoutput_row_filter(), so that two
> > different checks related to 'rfisnull' (introduced by different
> > patches) can be combined as if .. else check.
> >
> Fixed.
>
> On Thu, Nov 25, 2021 at 12:03 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > + * If the new relation or the old relation has a where clause,
> > + * we need to remove it so that it can be added afresh later.
> > + */
> > + if (RelationGetRelid(newpubrel->relation) == oldrelid &&
> > + newpubrel->whereClause == NULL && rfisnull)
> >
> > Can't we use _equalPublicationTable() here? It compares the whereClause as
> > well.
> >
>
> Tried this, can't do this because one is an alter statement while the
> other is a publication, the whereclause is not
> the same Nodetype. In the statement, the whereclause is T_A_Expr,
> while in the publication
> catalog, it is T_OpExpr.
Here we will not be able to do a direct comparison as we store the
transformed where clause in the pg_publication_rel table. We will have
to transform the where clause and then check. I have attached a patch
where we can check the transformed where clause and see if the where
clause is the same or not. If you are ok with this approach you could
make similar changes.
Regards,
Vignesh
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index b80c21e6ae..ae4a46e44a 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -359,6 +359,32 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt,
return result;
}
+Node *
+GetTransformedWhereClause(ParseState *pstate, PublicationRelInfo *pri,
+ bool bfixupcollation)
+{
+ ParseNamespaceItem *nsitem;
+ Node *transformedwhereclause = NULL;
+
+ pstate->p_sourcetext = nodeToString(pri->whereClause);
+
+ nsitem = addRangeTableEntryForRelation(pstate, pri->relation,
+ AccessShareLock,
+ NULL, false, false);
+ addNSItemToQuery(pstate, nsitem, false, true, true);
+
+ transformedwhereclause = transformWhereClause(pstate,
+ copyObject(pri->whereClause),
+ EXPR_KIND_PUBLICATION_WHERE,
+ "PUBLICATION WHERE");
+
+ /* Fix up collation information */
+ if (bfixupcollation)
+ assign_expr_collations(pstate, transformedwhereclause);
+
+ return transformedwhereclause;
+}
+
/*
* Insert new publication / relation mapping.
*/
@@ -377,7 +403,6 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
ObjectAddress myself,
referenced;
ParseState *pstate;
- ParseNamespaceItem *nsitem;
Node *whereclause = NULL;
List *relids = NIL;
@@ -408,20 +433,8 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
{
/* Set up a ParseState to parse with */
pstate = make_parsestate(NULL);
- pstate->p_sourcetext = nodeToString(pri->whereClause);
-
- nsitem = addRangeTableEntryForRelation(pstate, targetrel,
- AccessShareLock,
- NULL, false, false);
- addNSItemToQuery(pstate, nsitem, false, true, true);
-
- whereclause = transformWhereClause(pstate,
- copyObject(pri->whereClause),
- EXPR_KIND_PUBLICATION_WHERE,
- "PUBLICATION WHERE");
- /* Fix up collation information */
- assign_expr_collations(pstate, whereclause);
+ whereclause = GetTransformedWhereClause(pstate, pri, true);
/*
* Walk the parse-tree of this publication row filter expression and
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 1c792ed9cb..6106ec25d4 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -497,6 +497,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
List *rels = NIL;
Form_pg_publication pubform = (Form_pg_publication) GETSTRUCT(tup);
Oid pubid = pubform->oid;
+ Node *oldrelwhereclause = NULL;
/*
* It is quite possible that for the SET case user has not specified any
@@ -554,8 +555,13 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
ObjectIdGetDatum(pubid));
if (HeapTupleIsValid(rftuple))
{
- SysCacheGetAttr(PUBLICATIONRELMAP, rftuple, Anum_pg_publication_rel_prqual,
- &rfisnull);
+ Datum whereClauseDatum;
+
+ whereClauseDatum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple, Anum_pg_publication_rel_prqual,
+ &rfisnull);
+ if (!rfisnull)
+ oldrelwhereclause = stringToNode(TextDatumGetCString(whereClauseDatum));
+
ReleaseSysCache(rftuple);
}
@@ -569,11 +575,31 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
* If the new relation or the old relation has a where clause,
* we need to remove it so that it can be added afresh later.
*/
- if (RelationGetRelid(newpubrel->relation) == oldrelid &&
- newpubrel->whereClause == NULL && rfisnull)
+ if (RelationGetRelid(newpubrel->relation) == oldrelid)
{
- found = true;
- break;
+ if (rfisnull && !newpubrel->whereClause)
+ {
+ found = true;
+ break;
+ }
+
+ if (!rfisnull && newpubrel->whereClause)
+ {
+ ParseState *pstate = make_parsestate(NULL);
+ Node *whereclause;
+
+ whereclause = GetTransformedWhereClause(pstate,
+ newpubrel,
+ false);
+ if (equal(oldrelwhereclause, whereclause))
+ {
+ free_parsestate(pstate);
+ found = true;
+ break;
+ }
+
+ free_parsestate(pstate);
+ }
}
}
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index f698049633..8c63dd5f85 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -20,6 +20,7 @@
#include "catalog/genbki.h"
#include "catalog/objectaddress.h"
#include "catalog/pg_publication_d.h"
+#include "parser/parse_node.h"
/* ----------------
* pg_publication definition. cpp turns this into
@@ -142,7 +143,9 @@ extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
extern Oid get_publication_oid(const char *pubname, bool missing_ok);
extern char *get_publication_name(Oid pubid, bool missing_ok);
-
+extern Node *GetTransformedWhereClause(ParseState *pstate,
+ PublicationRelInfo *pri,
+ bool bfixupcollation);
extern bool check_rowfilter_replident(Node *node, Bitmapset *bms_replident);
#endif /* PG_PUBLICATION_H */
