Re: row filtering for logical replication

On Fri, Nov 5, 2021 at 10:44 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> PSA new set of v37* patches.
>

Few comments about changes made to the patch to rebase it:
1.
+#if 1
+ // FIXME - can we do a better job if integrating this with the schema changes
+ /*
+ * Remove all publication-table mappings.  We could possibly remove (i)
+ * tables that are not found in the new table list and (ii) tables that
+ * are being re-added with a different qual expression. For (ii),
+ * simply updating the existing tuple is not enough, because of qual
+ * expression dependencies.
+ */
+ foreach(oldlc, oldrelids)
+ {
+ Oid oldrelid = lfirst_oid(oldlc);
+ PublicationRelInfo *oldrel;
+
+ oldrel = palloc(sizeof(PublicationRelInfo));
+ oldrel->relid = oldrelid;
+ oldrel->whereClause = NULL;
+ oldrel->relation = table_open(oldrel->relid,
+   ShareUpdateExclusiveLock);
+ delrels = lappend(delrels, oldrel);
+ }
+#else
  CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
    PUBLICATIONOBJ_TABLE);

I think for the correct merge you need to just call
CheckObjSchemaNotAlreadyInPublication() before this for loop. BTW, I
have a question regarding this implementation. Here, it has been
assumed that the new rel will always be specified with a different
qual, what if there is no qual or if the qual is the same?

2.
+preprocess_pubobj_list(List *pubobjspec_list, core_yyscan_t
yyscanner, bool alter_drop)
 {
  ListCell   *cell;
  PublicationObjSpec *pubobj;
@@ -17341,7 +17359,15 @@ preprocess_pubobj_list(List *pubobjspec_list,
core_yyscan_t yyscanner)
  errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("invalid table name at or near"),
  parser_errposition(pubobj->location));
- else if (pubobj->name)
+
+ /* cannot use WHERE w-filter for DROP TABLE from publications */
+ if (pubobj->pubtable && pubobj->pubtable->whereClause && alter_drop)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid use of WHERE row-filter in ALTER PUBLICATION ... DROP TABLE"),
+ parser_errposition(pubobj->location));
+

This change looks a bit ad-hoc to me. Can we handle this at a later
point of time in publicationcmds.c?

3.
- | ColId
+ | ColId OptWhereClause
  {
  $$ = makeNode(PublicationObjSpec);
  $$->pubobjtype = PUBLICATIONOBJ_CONTINUATION;
- $$->name = $1;
+ if ($2)
+ {
+ $$->pubtable = makeNode(PublicationTable);
+ $$->pubtable->relation = makeRangeVar(NULL, $1, @1);
+ $$->pubtable->whereClause = $2;
+ }
+ else
+ {
+ $$->name = $1;
+ }

Again this doesn't appear to be the right way. I think this should be
handled at a later point.

-- 
With Regards,
Amit Kapila.