0002-review.patch

From a1cb64a17b35dd75daf22a9f59da176ed109612a Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Tue, 14 Dec 2021 22:11:10 +0100
Subject: [PATCH 2/2] review

---
 src/backend/catalog/pg_publication.c        | 60 +++++++++++++++------
 src/backend/commands/publicationcmds.c      | 12 +++++
 src/backend/commands/tablecmds.c            |  2 +-
 src/backend/replication/logical/proto.c     |  8 +++
 src/backend/replication/pgoutput/pgoutput.c |  8 +++
 5 files changed, 72 insertions(+), 18 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index ae58adc8e5..7a478b7072 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -99,28 +99,27 @@ check_publication_add_relation(Relation targetrel, Bitmapset *columns)
 	 */
 	if (columns != NULL)
 	{
+		Bitmapset  *idattrs;
+
 		if (replidentfull)
 			ereport(ERROR,
 					errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					errmsg("invalid column list for publishing relation \"%s\"",
 						   RelationGetRelationName(targetrel)),
 					errdetail("Cannot have column filter on relations with REPLICA IDENTITY FULL."));
-		else
-		{
-			Bitmapset  *idattrs;
-
-			idattrs = RelationGetIndexAttrBitmap(targetrel,
-												 INDEX_ATTR_BITMAP_IDENTITY_KEY);
-			if (!bms_is_subset(idattrs, columns))
-				ereport(ERROR,
-						errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
-						errmsg("invalid column list for publishing relation \"%s\"",
-							   RelationGetRelationName(targetrel)),
-						errdetail("All columns in REPLICA IDENTITY must be present in the column list."));
-
-			if (idattrs)
-				pfree(idattrs);
-		}
+
+		/* XXX The else was unnecessary, because the "if" always errors-out */
+		idattrs = RelationGetIndexAttrBitmap(targetrel,
+											 INDEX_ATTR_BITMAP_IDENTITY_KEY);
+		if (!bms_is_subset(idattrs, columns))
+			ereport(ERROR,
+					errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+					errmsg("invalid column list for publishing relation \"%s\"",
+						   RelationGetRelationName(targetrel)),
+					errdetail("All columns in REPLICA IDENTITY must be present in the column list."));
+
+		if (idattrs)
+			pfree(idattrs);
 	}
 }
 
@@ -352,6 +351,15 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 
 		/* FIXME need to handle the case of different column list */
 
+		/*
+		 * XXX So what's the right behavior for ADD TABLE with different column
+		 * list? I'd say we should allow that, and that it should be mostly the
+		 * same thing as adding/removing columns to the list incrementally, i.e.
+		 * we should replace the column lists. We could also prohibit, but that
+		 * seems like a really annoying limitation, forcing people to remove/add
+		 * the relation.
+		 */
+
 		if (if_not_exists)
 			return InvalidObjectAddress;
 
@@ -361,6 +369,10 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 						RelationGetRelationName(targetrel->relation), pub->name)));
 	}
 
+	/*
+	 * Translate list of columns to attnums. We prohibit system attributes and
+	 * make sure there are no duplicate columns.
+	 */
 	attarray = palloc(sizeof(AttrNumber) * list_length(targetrel->columns));
 	foreach(lc, targetrel->columns)
 	{
@@ -372,12 +384,23 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 					errcode(ERRCODE_UNDEFINED_COLUMN),
 					errmsg("column \"%s\" of relation \"%s\" does not exist",
 						   colname, RelationGetRelationName(targetrel->relation)));
-		if (attnum < 0)
+
+		if (!AttrNumberIsForUserDefinedAttr(attnum))
 			ereport(ERROR,
 					errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
 					errmsg("cannot reference system column \"%s\" in publication column list",
 						   colname));
 
+		/*
+		 * XXX The offset seems kinda pointless, because at this point we're not
+		 * dealing with system attributes, so all attnums are > 0. Are we comparing
+		 * it to arbitrary attnums later? This would simplify adding the deps later
+		 * because we're offsetting it back.
+		 *
+		 * XXX I wouldn't say "twice" though. It can be specified multiple times,
+		 * it's just that we discover it during the second occurrence. I'd say
+		 * "duplicate column name in publication column list" instead.
+		 */
 		if (bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber, attmap))
 			ereport(ERROR,
 					errcode(ERRCODE_DUPLICATE_OBJECT),
@@ -417,6 +440,9 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 	CatalogTupleInsert(rel, tup);
 	heap_freetuple(tup);
 
+	/* XXX not sure if worth an explicit free, but if we free the tuple ... */
+	pfree(attarray);
+
 	ObjectAddressSet(myself, PublicationRelRelationId, prrelid);
 
 	/* Add dependency on the publication */
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index a070914bdd..38efc5ad59 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -759,6 +759,8 @@ AlterPublication(ParseState *pstate, AlterPublicationStmt *stmt)
 /*
  * Remove relation from publication by mapping OID, or publication status
  * of one column of that relation in the publication if an attnum is given.
+ *
+ * XXX Whats "publication status"?
  */
 void
 RemovePublicationRelById(Oid proid, int32 attnum)
@@ -825,6 +827,9 @@ RemovePublicationRelById(Oid proid, int32 attnum)
 								 &isnull);
 		if (isnull)			/* shouldn't happen */
 			elog(ERROR, "can't drop column from publication without a column list");
+
+		/* XXX We're reading the array in multiple places, I suggest to move it
+		 * to a separate function, maybe? */
 		arr = DatumGetArrayTypeP(adatum);
 		nelems = ARR_DIMS(arr)[0];
 		elems = (int16 *) ARR_DATA_PTR(arr);
@@ -833,6 +838,7 @@ RemovePublicationRelById(Oid proid, int32 attnum)
 		newelems = palloc(sizeof(int16) * nelems - 1);
 		for (i = 0, j = 0; i < nelems - 1; i++)
 		{
+			/* XXX Can it happen that we never find a match? Seems like an error. */
 			if (elems[i] == attnum)
 				continue;
 			newelems[j++] = elems[i];
@@ -842,6 +848,10 @@ RemovePublicationRelById(Oid proid, int32 attnum)
 		 * If this is the last column used in the publication, disallow the
 		 * command. We could alternatively just drop the relation from the
 		 * publication.
+		 *
+		 * XXX Alternatively we could switch to "replicate all column", but
+		 * that seems counterintuitive. However, is there a way to switch
+		 * between these two modes, somehow?
 		 */
 		if (j == 0)
 		{
@@ -1154,6 +1164,8 @@ PublicationDropTables(Oid pubid, List *rels, bool missing_ok)
 		Relation	rel = pubrel->relation;
 		Oid			relid = RelationGetRelid(rel);
 
+		/* XXX Shouldn't this be prevented by the grammar, ideally? Can it actually
+		 * happen? It does not seem to be tested in the regression tests. */
 		if (pubrel->columns)
 			ereport(ERROR,
 					errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7207dcf9c0..f7c21158c0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8422,7 +8422,7 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
 	ReleaseSysCache(tuple);
 
 	/*
-	 * If the column is part of a replication column list, arrange to get that
+	 * If the column is part of a publication column list, arrange to get that
 	 * removed too.
 	 */
 	findAndAddAddresses(addrs, PublicationRelRelationId,
diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index 15d8192238..885acd6c9e 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -777,6 +777,13 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binar
 		 * Do not increment count of attributes if not a part of column
 		 * filters except for replica identity columns or if replica identity
 		 * is full.
+		 *
+		 * XXX This exact check is done in multiple places, so maybe let's move it
+		 * to a separate function and call it. Also, att_map is for user attrs only
+		 * so maybe we can get rid of the offsets?
+		 *
+		 * XXX Why do we need to check both att_map and idattrs? Aren't we enforcing
+		 * that indattrs is always a subset of att_map?
 		 */
 		if (att_map != NULL &&
 			!bms_is_member(att->attnum - FirstLowInvalidHeapAttributeNumber,
@@ -970,6 +977,7 @@ logicalrep_write_attrs(StringInfo out, Relation rel, Bitmapset *att_map)
 			continue;
 		}
 		/* Skip sending if not a part of column filter */
+		/* XXX How come this is not checking idattrs and replindentfull? */
 		if (att_map != NULL &&
 			!bms_is_member(att->attnum - FirstLowInvalidHeapAttributeNumber,
 						   att_map))
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index f9f9ecd0c0..551c4d5315 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -134,6 +134,14 @@ typedef struct RelationSyncEntry
 	 * having identical TupleDesc.
 	 */
 	TupleConversionMap *map;
+
+	/* XXX Needs a comment explaining what att_map does, how it's different
+	 * from map. The naming seems really confusing and it's a bit unclear how
+	 * these two bits interact (i.e. partitioning + column list).
+	 *
+	 * XXX In fact, is it really mapping anything? It seems more like a simple
+	 * list of attnums, translated from list of names.
+	 */
 	Bitmapset  *att_map;
 } RelationSyncEntry;
 
-- 
2.31.1