Re: Logical replication - schema change not invalidating the relation cache

[vignesh C]

On Fri, Jul 16, 2021 at 10:51 PM vignesh C  wrote:
>
> On Sat, Jul 3, 2021 at 11:23 AM Dilip Kumar  wrote:
> >
> > On Fri, Jul 2, 2021 at 12:03 PM Dilip Kumar  wrote:
> > >
> > > Yeah, this looks like a bug.  I will look at the patch.
> > >
> >
> > While looking into this, I think the main cause of the problem is that
> > schema rename does not invalidate the relation cache right?  I also
> > tried other cases e.g. if there is an open cursor and we rename the
> > schema
> >
> > CREATE SCHEMA sch1;
> > CREATE TABLE sch1.t1(c1 int);
> > insert into sch1.t1 values(1);
> > insert into sch1.t1 values(2);
> > insert into sch1.t1 values(3);
> > BEGIN;
> > DECLARE mycur CURSOR FOR SELECT * FROM sch1.t1;
> > FETCH NEXT FROM mycur ;
> > --At this point rename sch1 to sch2 from another session--
> > FETCH NEXT FROM mycur ;
> > UPDATE sch2.t1 SET c1 = 20 WHERE CURRENT OF mycur;
> > select * from sch2.t1 ;
> >
> > So even after the schema rename the cursor is able to fetch and its
> > also able to update on the same table in the new schema, ideally using
> > CURRENT OF CUR, you can update the same table for which you have
> > declared the cursor.  I am giving this example because this behavior
> > also looks somewhat similar.
>
> It works in this case because it uses the relation id for performing
> the next fetch and the relation id does not get changed after renaming
> the schema. Also since it holds a lock on the relation, alter/drop
> operations will not be allowed. I felt this behavior might be ok.  But
> the original scenario reported is an issue because it replicates the
> data of both the original table and the renamed schema's table.

The previous patch was failing because of the recent test changes made
by commit 201a76183e2 which unified new and get_new_node, attached
patch has the changes to handle the changes accordingly.

Regards,
Vignesh
From e86247f9502727f2c2e5f41489f8bbd4f69b24e4 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C 
Date: Thu, 26 Aug 2021 19:55:35 +0530
Subject: [PATCH v1] Fix for invalidating logical replication relations when
 there is a change in schema.

When the schema gets changed, the rel sync cache invalidation was not
happening, fixed it by adding a callback for schema change.
---
 src/backend/replication/pgoutput/pgoutput.c | 51 ++
 src/test/subscription/t/001_rep_changes.pl  | 76 -
 2 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 14d737fd93..1eec9f603d 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -141,6 +141,8 @@ static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data, Oid relid);
 static void rel_sync_cache_relation_cb(Datum arg, Oid relid);
 static void rel_sync_cache_publication_cb(Datum arg, int cacheid,
 		  uint32 hashvalue);
+static void rel_sync_cache_namespace_cb(Datum arg, int cacheid,
+		uint32 hashvalue);
 static void set_schema_sent_in_streamed_txn(RelationSyncEntry *entry,
 			TransactionId xid);
 static bool get_schema_sent_in_streamed_txn(RelationSyncEntry *entry,
@@ -1068,6 +1070,9 @@ init_rel_sync_cache(MemoryContext cachectx)
 	CacheRegisterSyscacheCallback(PUBLICATIONRELMAP,
   rel_sync_cache_publication_cb,
   (Datum) 0);
+	CacheRegisterSyscacheCallback(NAMESPACEOID,
+  rel_sync_cache_namespace_cb,
+  (Datum) 0);
 }
 
 /*
@@ -1342,6 +1347,52 @@ rel_sync_cache_relation_cb(Datum arg, Oid relid)
 	}
 }
 
+/*
+ * Namespace syscache invalidation callback
+ */
+static void
+rel_sync_cache_namespace_cb(Datum arg, int cacheid, uint32 hashvalue)
+{
+	HASH_SEQ_STATUS status;
+	RelationSyncEntry *entry;
+
+	/*
+	 * We can get here if the plugin was used in SQL interface as the
+	 * RelSchemaSyncCache is destroyed when the decoding finishes, but there
+	 * is no way to unregister the relcache invalidation callback.
+	 */
+	if (RelationSyncCache == NULL)
+		return;
+
+	hash_seq_init(, RelationSyncCache);
+	while ((entry = (RelationSyncEntry *) hash_seq_search()) != NULL)
+	{
+		/*
+		 * Reset schema sent status as the relation definition may have changed.
+		 * Also free any objects that depended on the earlier definition.
+		 */
+		entry->schema_sent = false;
+		list_free(entry->streamed_txns);
+		entry->streamed_txns = NIL;
+		if (entry->map)
+		{
+			/*
+			 * Must free the TupleDescs contained in the map explicitly,
+			 * because free_conversion_map() doesn't.
+			 */
+			FreeTupleDesc(entry->map->indesc);
+			FreeTupleDesc(entry->map->outdesc);
+			free_conversion_map(entry->map);
+		}
+		entry->map = NULL;
+
+		if (hash_search(RelationSyncCache,

Re: Added schema level support for publication.

[vignesh C]

On Sat, Aug 28, 2021 at 3:19 PM Amit Kapila  wrote:
>
> On Fri, Aug 27, 2021 at 6:09 PM vignesh C  wrote:
> >
> > On Fri, Aug 27, 2021 at 4:57 PM Amit Kapila  wrote:
> > >
> > > On Fri, Aug 27, 2021 at 11:43 AM vignesh C  wrote:
> > > >
> > > > On Wed, Aug 25, 2021 at 3:07 PM vignesh C  wrote:
> > > >
> > > > I have implemented this in the 0003 patch, I have kept it separate to
> > > > reduce the testing effort and also it will be easier if someone
> > > > disagrees with the syntax. I will merge it to the main patch later
> > > > based on the feedback. Attached v22 patch has the changes for the
> > > > same.
> > > >
> > >
> > > Few comments on v22-0001-Added-schema-level-support-for-publication:
> > > 
> > > 1. Why in publication_add_schema(), you are registering invalidation
> > > for all schema relations? It seems this is to allow rebuilding the
> > > publication info for decoding sessions. But that is not required as
> > > you are registering rel_sync_cache_publication_cb for
> > > publication_schema relation. In rel_sync_cache_publication_cb, we are
> > > marking replicate_valid as false for each entry which will allow
> > > publication info to be rebuilt in get_rel_sync_entry.
> > >
> > > I see that it is done for a single relation in the current code in
> > > function publication_add_relation but I guess that is also not
> > > required. You can once test this. If you still think it is required,
> > > can you please explain me why and then we can accordingly add some
> > > comments in the patch.
> >
> > I felt this is required for handling the following concurrency scenario:
> > create schema sch1;
> > create table sch1.t1(c1 int);
> > insert into sch1.t1 values(10);
> > update sch1.t1 set c1 = 11;
> > # update will be successful and relation cache will update publication
> > actions based on the current state i.e no publication
> > create publication pub1 for all tables in schema sch1;
> > # After the above publication is created the relations present in this
> > schema should be invalidated so that the next update should fail.
> >
>
> What do you mean by update should fail? I think all the relations in
> RelationSyncCache via rel_sync_cache_publication_cb because you have
> updated pg_publication_schema and that should register syscache
> invalidation.

By update should fail, I meant the updates without setting replica
identity before creating the decoding context. The scenario is like
below (all in the same session, the subscription is not created):
create schema sch1;
create table sch1.t1(c1 int);
insert into sch1.t1 values(10);
# Before updating we will check CheckCmdReplicaIdentity, as there are
no publications on this table rd_pubactions will be set accordingly in
relcache entry.
update sch1.t1 set c1 = 11;
# Now we will create the publication after rd_pubactions has been set
in the cache. Now when we create this publication we should invalidate
the relations present in the schema, this is required so that when the
next update happens, we should check the publication actions again in
CheckCmdReplicaIdentity and fail the update which does not  set
replica identity after the publication is created.
create publication pub1 for all tables in schema sch1;
# After the above publication is created the relations present in this
schema will be invalidated. Now we will check the publication actions
again in CheckCmdReplicaIdentity and the update will fail.
update sch1.t1 set c1 = 11;
ERROR:  cannot update table "t1" because it does not have a replica
identity and publishes updates
HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.

The rel_sync_cache_publication_cb invalidations are registered once
the decoding context is created. Since the decoding context is not
created at this point of time we should explicitly invalidate all the
relations present in this schema by calling InvalidatePublicationRels.

> > If
> > the relations are not invalidated the updates will be successful based
> > on previous publication actions.
> > update sch1.t1 set c1 = 11;
> >
>
> I think even without special relcache invalidations the relations
> should be invalidated because of syscache invalidation as mentioned in
> the previous point. Am I missing something here?

The rel_sync_cache_publication_cb invalidations are registered once
the decoding context is created. Since the decoding context is not
created at this point of time we should explicitly invalidate all the
relations present in this schema by c

Re: Added missing invalidations for all tables publication

[vignesh C]

On Tue, Aug 31, 2021 at 7:40 AM houzj.f...@fujitsu.com
 wrote:
>
> From Tuesday, August 31, 2021 1:10 AM vignesh C  wrote:
> > Hi,
> >
> > Relation invalidation was missing in case of create publication and drop
> > publication of "FOR ALL TABLES" publication, added so that the publication
> > information can be rebuilt. Without these invalidation update/delete
> > operations on the relation will be successful in the publisher which will 
> > later
> > result in conflict in the subscriber.
> > Thanks to Amit for identifying the issue at [1]. Attached patch has the fix 
> > for the
> > same.
> > Thoughts?
>
> I have one comment about the testcase in the patch.
>
> +-- Test cache invalidation FOR ALL TABLES publication
> +SET client_min_messages = 'ERROR';
> +CREATE TABLE testpub_tbl4(a int);
> +CREATE PUBLICATION testpub_foralltables FOR ALL TABLES;
> +RESET client_min_messages;
> +-- fail missing REPLICA IDENTITY
> +UPDATE testpub_tbl4 set a = 2;
> +ERROR:  cannot update table "testpub_tbl4" because it does not have a 
> replica identity and publishes updates
> +HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
> +DROP PUBLICATION testpub_foralltables;
>
> The above testcases can pass without the code change in the patch, is it 
> better
> to add a testcase which can show different results after applying the patch ?

Thanks for the comment, I have slightly modified the test case which
will fail without the patch. Attached v2 patch which has the changes
for the same.

Regards,
Vignesh
From 3b7fc6828efa979cbf8a89f47d3c4e4f1fb5041a Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Mon, 30 Aug 2021 22:29:07 +0530
Subject: [PATCH v2] Added missing invalidations for all tables publication.

Relation invalidation was missing in case of create publication and drop
publication of "FOR ALL TABLES" publication, added them so that the
publication information can be rebuilt.
---
 src/backend/catalog/dependency.c  |  5 +++-
 src/backend/commands/publicationcmds.c| 33 +++
 src/include/commands/publicationcmds.h|  1 +
 src/test/regress/expected/publication.out | 15 +++
 src/test/regress/sql/publication.sql  | 14 ++
 5 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 76b65e39c4..91c3e976e0 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1460,6 +1460,10 @@ doDeletion(const ObjectAddress *object, int flags)
 			RemovePublicationRelById(object->objectId);
 			break;
 
+		case OCLASS_PUBLICATION:
+			RemovePublicationById(object->objectId);
+			break;
+
 		case OCLASS_CAST:
 		case OCLASS_COLLATION:
 		case OCLASS_CONVERSION:
@@ -1478,7 +1482,6 @@ doDeletion(const ObjectAddress *object, int flags)
 		case OCLASS_USER_MAPPING:
 		case OCLASS_DEFACL:
 		case OCLASS_EVENT_TRIGGER:
-		case OCLASS_PUBLICATION:
 		case OCLASS_TRANSFORM:
 			DropObjectById(object);
 			break;
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 8487eeb7e6..54eb0b20b4 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -234,6 +234,9 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
 		PublicationAddTables(puboid, rels, true, NULL);
 		CloseTableList(rels);
 	}
+	/* Invalidate relcache so that publication info is rebuilt. */
+	else if (stmt->for_all_tables)
+		CacheInvalidateRelcacheAll();
 
 	table_close(rel, RowExclusiveLock);
 
@@ -497,6 +500,36 @@ RemovePublicationRelById(Oid proid)
 	table_close(rel, RowExclusiveLock);
 }
 
+/*
+ * Remove the publication by mapping OID.
+ */
+void
+RemovePublicationById(Oid pubid)
+{
+	Relation	rel;
+	HeapTuple	tup;
+	Form_pg_publication pubform;
+
+	rel = table_open(PublicationRelationId, RowExclusiveLock);
+
+	tup = SearchSysCache1(PUBLICATIONOID, ObjectIdGetDatum(pubid));
+	if (!HeapTupleIsValid(tup))
+		elog(ERROR, "cache lookup failed for publication %u",
+			 pubid);
+
+	pubform = (Form_pg_publication) GETSTRUCT(tup);
+
+	/* Invalidate relcache so that publication info is rebuilt. */
+	if (pubform->puballtables)
+		CacheInvalidateRelcacheAll();
+
+	CatalogTupleDelete(rel, >t_self);
+
+	ReleaseSysCache(tup);
+
+	table_close(rel, RowExclusiveLock);
+}
+
 /*
  * Open relations specified by a RangeVar list.
  * The returned tables are locked in ShareUpdateExclusiveLock mode in order to
diff --git a/src/include/commands/publicationcmds.h b/src/include/commands/publicationcmds.h
index a3fa2ac6cd..c98d519b29 100644
--- a/src/include/commands/publicationcmds.h
+++ b/src/include/commands/publicationcmds.h
@@ -20,6 +20,7 @@
 
 extern ObjectAddress CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)

Re: Added schema level support for publication.

[vignesh C]

On Mon, Aug 30, 2021 at 2:14 PM Greg Nancarrow  wrote:
>
> On Fri, Aug 27, 2021 at 4:13 PM vignesh C  wrote:
> >
> > I have implemented this in the 0003 patch, I have kept it separate to
> > reduce the testing effort and also it will be easier if someone
> > disagrees with the syntax. I will merge it to the main patch later
> > based on the feedback. Attached v22 patch has the changes for the
> > same.
>
> I notice that "CREATE PUBLICATION pub1 FOR ALL TABLES IN SCHEMA sc1,
> TABLE sc1.test;"  maintains the table separately and results in the
> following in the \dRp+ output:
>
> Tables:
> "sc1.test"
> Schemas:
> "sc1"
>
> and also then "ALTER PUBLICATION pub1 DROP ALL TABLES IN SCHEMA sc1;"
> still leaves the "sc1.test" table in the publication.

I had intentionally implemented this way, the reason being it gives
the flexibility to modify the publications based on the way the
publication is created. My idea was that if a user specified a
table/schema of the same schema while creating the publication, the
user should be allowed to drop any of them at any time. In the above
case if we don't maintain the results separately, users will not be
able to drop the table from the publication at a later point of time.
Thoughts?

> Is there a reason why we don't/can't support "ALTER SUBSCRIPTION ...
> SET ALL TABLES;"?
> (I know it wasn't supported before, but now "ALTER SUBSCRIPTION ...
> SET ALL TABLES IN SCHEMA ..." is being supported)

I agree this can be implemented with the current design. I felt I can
work on getting the current patches into a committable shape and then
work on this after the current patch series is finished. Thoughts?

> I notice that the v22-0003 documentation updates for ALTER
> SUBSCRIPTION are missing - but you're probably waiting on all feedback
> before proceeding with that.

I have fixed this as part of v23 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm0xmqJeQEfV5Wnj2BawM%3DsdFdfOXz5N%2BgGG3WB6k9%3Dtdw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Aug 30, 2021 at 12:12 PM Amit Kapila 
wrote:
>
>
> Okay, I got it but let's add few comments in the code related to it.
> Also, I noticed that the code in InvalidatePublicationRels() already
> exists in AlterPublicationOptions(). You can try to refactor the
> existing code as a separate initial patch.

I have made these changes at the v23 patch attached at [1].

> BTW, I noticed that "for all tables", we don't register invalidations
> in the above scenario, and then later that causes conflict on the
> subscriber. I think that is a bug in the current code and we can deal
> with that separately.

I agree that the cache invalidation has been missed in case of "for all
tables" publication, I have fixed these and posted a patch for the same at
[2].

[1] -
https://www.postgresql.org/message-id/CALDaNm0xmqJeQEfV5Wnj2BawM%3DsdFdfOXz5N%2BgGG3WB6k9%3Dtdw%40mail.gmail.com
[2] -
https://www.postgresql.org/message-id/CALDaNm0zkQznFrxzHBoWZUGsf%3DnKSxhEZZhZ1eTDWLpFok6zZw%40mail.gmail.com

Regards,
Vigneshhas

Re: Added schema level support for publication.

[vignesh C]

On Tue, Aug 10, 2021 at 1:40 PM Greg Nancarrow  wrote:
>
> On Fri, Aug 6, 2021 at 6:32 PM vignesh C  wrote:
> >
> > Thanks for the comments, the attached v19 patch has the fixes for the
comments.
> >
>
> Some more review comments, this time for the v19 patch:
>
>
> (1) In patch v19-0002, there's still a couple of instances where it
> says "publication type" instead of "publication kind".

Modified

> (2) src/backend/catalog/pg_publication.c
>
> "This should only be used for normal publications."
>
> What exactly is meant by that - what type is considered normal? Maybe
> that comment should be more specific.

Modified

> (3) src/backend/catalog/pg_publication.c
> GetSchemaPublications
>
> Function header says "Gets list of publication oids for publications
> marked as FOR SCHEMA."
>
> Shouldn't it say something like: "Gets the list of FOR SCHEMA
> publication oids associated with a specified schema oid." or something
> like that?
> (since the function accepts a schemaid parameter)

Modfified

> (4) src/backend/commands/publicationcmds.c
>
> In AlterPublicationSchemas(), I notice that the two error cases
> "cannot be added to or dropped ..." don't check stmt->action for
> DEFELEM_ADD/DEFELEM_DROP.
> Is that OK? (i.e. should these cases error out if stmt->action is not
> DEFELEM_ADD/DEFELEM_DROP?)
> Also, I assume that the else part (not DEFELEM_ADD/DEFELEM_DROP) is
> the "Set" case? Maybe a comment should be added to the top of the else
> part.

The error message should also include set, I have modified the error
message accordingly.

> (5) src/backend/commands/publicationcmds.c
> Typo (same in 2 places): "automaically" -> "automatically"
>
> +  * will be released automaically at the end of create publication
>
> See functions:
> (i) CreatePublication
> (ii) AlterPublicationSchemas

Modified.

> (6) src/backend/commands/publicationcmds.c
> LockSchemaList
>
> Function header says "are locked in ShareUpdateExclusiveLock mode" but
> then code calls LockDatabaseObject using "AccessShareLock".

Modified.

Thanks for the comments, these issues are fixed in the v20 patch attached
at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Aug 9, 2021 at 9:50 PM Mark Dilger 
wrote:
>
>
>
> > On Aug 6, 2021, at 1:32 AM, vignesh C  wrote:
> >
> > the attached v19 patch
>
> With v19 applied, a schema owner can publish the contents of a table
regardless of ownership or permissions on that table:
>
> +CREATE ROLE user1;
> +GRANT CREATE ON DATABASE regression TO user1;
> +CREATE ROLE user2;
> +GRANT CREATE ON DATABASE regression TO user2;
> +SET SESSION AUTHORIZATION user1;
> +CREATE SCHEMA user1schema;
> +GRANT CREATE, USAGE ON SCHEMA user1schema TO user2;
> +RESET SESSION AUTHORIZATION;
> +SET SESSION AUTHORIZATION user2;
> +CREATE TABLE user1schema.user2private (junk text);
> +REVOKE ALL PRIVILEGES ON user1schema.user2private FROM PUBLIC;
> +REVOKE ALL PRIVILEGES ON user1schema.user2private FROM user1;
> +CREATE TABLE user1schema.user2public (junk text);
> +GRANT SELECT ON user1schema.user2public TO PUBLIC;
> +RESET SESSION AUTHORIZATION;
> +SET SESSION AUTHORIZATION user1;
> +SELECT * FROM user1schema.user2private;
> +ERROR:  permission denied for table user2private
> +SELECT * FROM user1schema.user2public;
> + junk
> +--
> +(0 rows)
> +
> +CREATE PUBLICATION user1pub;
> +WARNING:  wal_level is insufficient to publish logical changes
> +HINT:  Set wal_level to logical before creating subscriptions.
> +ALTER PUBLICATION user1pub
> +   ADD TABLE user1schema.user2public;
> +ERROR:  must be owner of table user2public
> +ALTER PUBLICATION user1pub
> +   ADD TABLE user1schema.user2private, user1schema.user2public;
> +ERROR:  must be owner of table user2private
> +SELECT * FROM pg_catalog.pg_publication_tables
> +   WHERE pubname = 'user1pub';
> + pubname | schemaname | tablename
> +-++---
> +(0 rows)
> +
> +ALTER PUBLICATION user1pub ADD SCHEMA user1schema;
> +SELECT * FROM pg_catalog.pg_publication_tables
> +   WHERE pubname = 'user1pub';
> + pubname  | schemaname  |  tablename
> +--+-+--
> + user1pub | user1schema | user2private
> + user1pub | user1schema | user2public
> +(2 rows)
>
> It is a bit counterintuitive that schema owners do not have
administrative privileges over tables within their schemas, but that's how
it is.  The design of this patch seems to assume otherwise.  Perhaps ALTER
PUBLICATION ... ADD SCHEMA should be restricted to superusers, just as FOR
ALL TABLES?

Thanks for the comment, this is handled in the v20 patch attached at [1].

> Alternatively, you could add ownership checks per table to mirror the
behavior of ALTER PUBLICATION ... ADD TABLE, but that would foreclose the
option of automatically updating the list of tables in the publication as
new tables are added to the schema, since those new tables would not
necessarily belong to the schema owner, and having a error thrown during
CREATE TABLE would be quite unfriendly.  I think until this is hammered
out, it is safer to require superuser privileges and then we can revisit
this issue and loosen the requirement in a subsequent commit.

I agree with Amit on this point for handling this as a separate patch, I
have not made any change for this, kept the behavior as is.

[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Thu, Aug 12, 2021 at 5:54 PM Masahiko Sawada 
wrote:
>
> On Fri, Aug 6, 2021 at 5:33 PM vignesh C  wrote:
> >
> > Thanks for the comments, the attached v19 patch has the fixes for the
comments.
>
> Thank you for updating the patch!
>
> Here are some comments on v19 patch:
>
> +case OCLASS_PUBLICATION_SCHEMA:
> +RemovePublicationSchemaById(object->objectId);
> +break;
>
> +void
> +RemovePublicationSchemaById(Oid psoid)
> +{
> +Relation   rel;
> +HeapTuple  tup;
> +
> +rel = table_open(PublicationSchemaRelationId, RowExclusiveLock);
> +
> +tup = SearchSysCache1(PUBLICATIONSCHEMA,
ObjectIdGetDatum(psoid));
> +
> +if (!HeapTupleIsValid(tup))
> +elog(ERROR, "cache lookup failed for publication schema
%u",
> + psoid);
> +
> +CatalogTupleDelete(rel, >t_self);
> +
> +ReleaseSysCache(tup);
> +
> +table_close(rel, RowExclusiveLock);
> +}
>
> Since RemovePublicationSchemaById() does simple catalog tuple
> deletion, it seems to me that we can DropObjectById() to delete the
> row of pg_publication_schema.

Relation cache invalidations were missing in the function, I have added and
retained the function with invalidation changes.

>  ---
>  {
> -ScanKeyInit([0],
> +ScanKeyData skey[1];
> +
> +ScanKeyInit([0],
>  Anum_pg_class_relkind,
>  BTEqualStrategyNumber, F_CHAREQ,
>
> CharGetDatum(RELKIND_PARTITIONED_TABLE));
>
> -scan = table_beginscan_catalog(classRel, 1, key);
> +scan = table_beginscan_catalog(classRel, 1, skey);
>
> Since we specify 1 as the number of keys in table_beginscan_catalog(),
> can we reuse 'key' instead of using 'skey'?

Modified.

> ---
> Even if we drop all tables added to the publication from it, 'pubkind'
> doesn't go back to 'empty'. Is that intentional behavior? If we do
> that, we can save the lookup of pg_publication_rel and
> pg_publication_schema in some cases, and we can switch the publication
> that was created as FOR SCHEMA to FOR TABLE and vice versa.

I felt this can be handled as a separate patch as the same scenario applies
for all tables publication too. Thoughts?

> ---
> +static void
> +UpdatePublicationKindTupleValue(Relation rel, HeapTuple tup, int col,
> +char
pubkind)
>
> Since all callers of this function specify Anum_pg_publication_pubkind
> to 'col', it seems 'col' is not necessary.

Modified

> ---
> +static void
> +AlterPublicationSchemas(AlterPublicationStmt *stmt, Relation rel,
> +HeapTuple tup,
> Form_pg_publication pubform)
>
> I think we don't need to pass 'pubform' to this function since we can
> get it by GETSTRUCT(tup).

Modified.

> ---
> +OidschemaId =
get_rel_namespace(relid);
>  List  *pubids = GetRelationPublications(relid);
> +List  *schemaPubids =
GetSchemaPublications(schemaId);
>
> Can we defer to get the list of schema publications (i.g.,
> 'schemaPubids') until we find the PUBKIND_SCHEMA publication? Perhaps
> the same is true for building 'pubids'.

I felt that we can get the publication information and use it whenever
required instead of querying in the loop. Thoughts?

> ---
> +   List of publications
> +Name|  Owner   | All tables | Inserts
> | Updates | Deletes | Truncates | Via root | PubKind
>
++--++-+-+-+---+--+-
> + testpib_ins_trunct | regress_publication_user | f  | t
> | f   | f   | f | f| e
> + testpub_default| regress_publication_user | f  | f
> | t   | f   | f | f| e
>
> I think it's more readable if \dRp shows 'all tables', 'table',
> 'schema', and 'empty' in PubKind instead of the single character.

Modified

> I think 'Pub kind' is more consistent with other column names.

Modified

Thanks for the comments, these issues are fixed in the v20 patch attached
at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Aug 9, 2021 at 10:23 AM Amit Kapila  wrote:
>
> On Sun, Aug 8, 2021 at 2:52 PM vignesh C  wrote:
> >
> > On Fri, Aug 6, 2021 at 4:39 PM Amit Kapila 
wrote:
> > >
> > > On Fri, Aug 6, 2021 at 2:02 PM vignesh C  wrote:
> > > >
> > > > On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila 
wrote:
> > > > >
> > > > > On Tue, Aug 3, 2021 at 8:38 PM vignesh C 
wrote:
> > > >
> > > > > 6.
> > > > > + {PublicationSchemaRelationId, /* PUBLICATIONSCHEMAMAP */
> > > > > + PublicationSchemaPsnspcidPspubidIndexId,
> > > > > + 2,
> > > > > + {
> > > > > + Anum_pg_publication_schema_psnspcid,
> > > > > + Anum_pg_publication_schema_pspubid,
> > > > > + 0,
> > > > > + 0
> > > > > + },
> > > > >
> > > > > Why don't we keep pubid as the first column in this index?
> > > >
> > > > I wanted to keep it similar to PUBLICATIONRELMAP, should we keep it
as
> > > > it is, thoughts?
> > > >
> > >
> > > Okay, I see your point. I think for PUBLICATIONRELMAP, we need it
> > > because it is searched using the only relid in
> > > GetRelationPublications, so, similarly, in the patch, you are using
> > > schema_oid in GetSchemaPublications, so probably that will help. I was
> > > wondering why you haven't directly used the cache in
> > > GetSchemaPublications similar to GetRelationPublications?
> >
> > Both of the approaches work, I was not sure which one is better, If
> > the approach in GetRelationPublications is better, I will change it to
> > something similar to GetRelationPublications. Thoughts?
> >
>
> I think it is better to use the cache as if we don't find the entry in
> the cache, then we will anyway search the required entry via sys table
> scan, see SearchCatCacheList.

Modified. This is handled in the v20 patch posted at [1].

I think the point I wanted to ensure was
> that a concurrent session won't blow up the entry while we are looking
> at it. How is that ensured?

The concurrency points occur at two places, Walsender session and user
session:
For Walsender process when we query the data from the cache we will get the
results based on historic snapshot. I also debugged and verified that we
get the results based on historic snapshot, if we check the cache during
our operation (insert is just before drop) we will be able to get the
dropped record from the cache as this drop occurred after our insert. And
if we query the cache after the drop, we will not get the dropped
information from the cache. So I feel our existing code is good enough
which handles the concurrency through the historic snapshot.
For user sessions, user session checks for replica identity for
update/delete operations. To prevent concurrency issues, when schema is
added to the publication, the rel cache invalidation happens in
publication_add_schema by calling InvalidatePublicationRels, similarly when
a schema is dropped from the publication, the rel cache invalidation is
handled in RemovePublicationSchemaById by calling
InvalidatePublicationRels. Once the invalidation happens it will check the
system tables again before deciding. I felt this rel cache invalidation
will prevent concurrency issues.

[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Aug 6, 2021 at 4:02 PM Amit Kapila  wrote:
>
> On Fri, Aug 6, 2021 at 2:16 PM vignesh C  wrote:
> >
> > On Thu, Aug 5, 2021 at 3:54 PM Amit Kapila 
wrote:
> > >
> > >
> > > Few more comments:
> > > ===
> > > 1. Do we need the previous column 'puballtables' after adding pubtype
> > > or pubkind in pg_publication?

Removed puballtables, this is handled as part of v20 patch attached at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 1, 2021 at 11:14 AM tanghy.f...@fujitsu.com
 wrote:
>
> > On Monday, August 30, 2021 11:28 PM vignesh C  wrote:
> >
> > I have fixed these comments as part of v23 patch attached at [1].
> > [1] - https://www.postgresql.org/message-
> > id/CALDaNm0xmqJeQEfV5Wnj2BawM%3DsdFdfOXz5N%2BgGG3WB6k9%3Dtdw
> > %40mail.gmail.com
> >
>
> Thanks for your new patch. Here are some comments on v23 patch.
>
> 1. doc/src/sgml/ref/alter_publication.sgml
> +  
> +   Add some schemas to the publication:
> +
> +ALTER PUBLICATION sales_publication ADD ALL TABLES IN SCHEMA marketing_june, 
> sales_june;
> +
> +  
>
> This change seems to be added twice, both 0003 and 0004 patch have this 
> change.

Modified

> 2. src/sgml/ref/create_publication.sgml
> There is the following description about "FOR TABLE" parameter:
>   Only persistent base tables and partitioned tables can be part of a
>   publication.  Temporary tables, unlogged tables, foreign tables,
>   materialized views, and regular views cannot be part of a publication.
>
> "FOR ALL TABLES IN SCHEMA" parameter also have restrictions, should we add
> some doc description for it?

Modified

> 3. When using '\dn+', I noticed that the list of publications only contains 
> the
> publications for "SCHEMA", "FOR ALL TABLES" publications are not shown. Is it 
> designed on purpose?
> (The result of '\d+' lists the publications of "SCHEAME" and "FOR ALL 
> TABLES").
>
> For example:
> create schema sch1;
> create table sch1.tbl(a int);
> create publication pub_schema for all tables in schema sch1;
> create publication pub_all_tables for all tables;

I'm not sure if it is intentional or not, Do you want to post the
question in a separate thread and see if that should be handled?

Thanks for the comments, the v24 patch attached at [1] handles the comments.
[1] - 
https://www.postgresql.org/message-id/CALDaNm27bs40Rxpy4oKfV97UgsPG%3DvVoZ5bj9pP_4BxnO-6DYA%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 1, 2021 at 6:58 AM houzj.f...@fujitsu.com
 wrote:
>
> Here are some other comments for v23-000x patches.
>
> 1)
>
> @@ -6225,6 +6342,9 @@ describePublications(const char *pattern)
> boolhas_pubtruncate;
> boolhas_pubviaroot;
>
> +   PQExpBufferData title;
> +   printTableContent cont;
> +
> if (pset.sversion < 10)
> {
> ...
> PQExpBufferData title;
> printTableOpt myopt = pset.popt.topt;
> printTableContent cont;
>
> Should we delete the inner declaration of 'title' and 'cont' ?

Modified

> 2)
> -   /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */
> +   /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE/SCHEMA */
>
> SCHEMA => ALL TABLES IN SCHEMA

Modified

> 3)
>
> + .description = 
> "PUBLICATION SCHEMA",
> + .section = 
> SECTION_POST_DATA,
> + .createStmt = 
> query->data));
>
> Is it better to use something like 'PUBLICATION TABLES IN SCHEMA' to describe
> the schema level table publication ? Because there could be some other type
> publication such as 'ALL SEQUENCES IN SCHEMA' in the future, it will be better
> to make it clear that we only publish table in schema in this patch.

Modified

Thanks for the comments, the v24 patch attached at [1] handles the comments.
[1] - 
https://www.postgresql.org/message-id/CALDaNm27bs40Rxpy4oKfV97UgsPG%3DvVoZ5bj9pP_4BxnO-6DYA%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Sat, Aug 28, 2021 at 3:19 PM Amit Kapila  wrote:
>
> Don't you think some users might want to know all the schema names for
> a publication? I am not completely sure on this point but I think it
> is good to have information for users. It might be also useful to have
> pg_publication_objects where we can display object types (like table,
> schema, sequence, etc) and then object names. If you are not convinced
> then we can wait and see what others think about this.

Thanks for the comment, this is handled in the v24 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm27bs40Rxpy4oKfV97UgsPG%3DvVoZ5bj9pP_4BxnO-6DYA%40mail.gmail.com

Regards,
Vignesh

Re: Column Filtering in Logical Replication

[vignesh C]

On Wed, Sep 15, 2021 at 5:20 PM Alvaro Herrera  wrote:
>
> On 2021-Sep-15, Amit Kapila wrote:
>
> > On Mon, Sep 6, 2021 at 11:21 PM Alvaro Herrera  
> > wrote:
> > >
> > > I pushed the clerical part of this -- namely the addition of
> > > PublicationTable node and PublicationRelInfo struct.
> >
> > One point to note here is that we are developing a generic grammar for
> > publications where not only tables but other objects like schema,
> > sequences, etc. can be specified, see [1]. So, there is some overlap
> > in the grammar modifications being made by this patch and the work
> > being done in that other thread.
>
> Oh rats.  I was not aware of that thread, or indeed of the fact that
> adding multiple object types to publications was being considered.
>
> I do see that 0002 there contains gram.y changes, but AFAICS those
> changes don't allow specifying a column list for a table, so there are
> some changes needed in that patch for that either way.
>
> I agree that it's better to move forward in unison.
>
> I noticed that 0002 in that other patch uses a void * pointer in
> PublicationObjSpec that "could be either RangeVar or String", which
> strikes me as a really bad idea.  (Already discussed in some other
> thread recently, maybe this one or the row filtering one.)

I have extracted the parser code and attached it here, so that it will
be easy to go through. We wanted to support the following syntax as in
[1]:
CREATE PUBLICATION pub1 FOR
TABLE t1,t2,t3, ALL TABLES IN SCHEMA s1,s2,
SEQUENCE seq1,seq2, ALL SEQUENCES IN SCHEMA s3,s4;

Columns can be added to PublicationObjSpec data structure. The patch
Generic_object_type_parser_002_table_schema_publication.patch has the
changes that were used to handle the parsing. Schema and Relation both
are different objects, schema is of string type and relation is of
RangeVar type. While parsing, schema name is parsed in string format
and relation is parsed and converted to rangevar type, these objects
will be then handled accordingly during post processing. That is the
reason it used void * type which could hold both RangeVar and String.
Thoughts?

[1] - https://www.postgresql.org/message-id/877603.1629120678%40sss.pgh.pa.us

Regards,
Vignesh
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index d6fddd6efe..2a2fe03c13 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -141,14 +141,14 @@ pg_relation_is_publishable(PG_FUNCTION_ARGS)
  * Insert new publication / relation mapping.
  */
 ObjectAddress
-publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
+publication_add_relation(Oid pubid, Relation targetrel,
 		 bool if_not_exists)
 {
 	Relation	rel;
 	HeapTuple	tup;
 	Datum		values[Natts_pg_publication_rel];
 	bool		nulls[Natts_pg_publication_rel];
-	Oid			relid = RelationGetRelid(targetrel->relation);
+	Oid			relid = RelationGetRelid(targetrel);
 	Oid			prrelid;
 	Publication *pub = GetPublication(pubid);
 	ObjectAddress myself,
@@ -172,10 +172,10 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 		ereport(ERROR,
 (errcode(ERRCODE_DUPLICATE_OBJECT),
  errmsg("relation \"%s\" is already member of publication \"%s\"",
-		RelationGetRelationName(targetrel->relation), pub->name)));
+		RelationGetRelationName(targetrel), pub->name)));
 	}
 
-	check_publication_add_relation(targetrel->relation);
+	check_publication_add_relation(targetrel);
 
 	/* Form a tuple. */
 	memset(values, 0, sizeof(values));
@@ -209,7 +209,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 	table_close(rel, RowExclusiveLock);
 
 	/* Invalidate relcache so that publication info is rebuilt. */
-	CacheInvalidateRelcache(targetrel->relation);
+	CacheInvalidateRelcache(targetrel);
 
 	return myself;
 }
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 30929da1f5..f5fd463c15 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -34,6 +34,7 @@
 #include "commands/publicationcmds.h"
 #include "funcapi.h"
 #include "miscadmin.h"
+#include "nodes/makefuncs.h"
 #include "utils/acl.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
@@ -138,6 +139,34 @@ parse_publication_options(ParseState *pstate,
 	}
 }
 
+/*
+ * Convert the PublicationObjSpecType list into rangevar list.
+ */
+static void
+ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
+		   List **rels)
+{
+	ListCell   *cell;
+	PublicationObjSpec *pubobj;
+
+	if (!pubobjspec_list)
+		return;
+
+	foreach(cell, pubobjspec_list)
+	{
+		Node *node;
+
+		pubobj = (PublicationObjSpec *) lfirst(cell);
+		node = (Node *) pubobj->object;
+
+		if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE)
+		{
+			if (IsA(node, RangeVar))
+*rels = lappend(*rels, (RangeVar *) node);
+		}		
+	}
+}
+ 
 /*
  * Create new publication.
  */
@@ -155,6 +184,7 @@ CreatePublication(ParseState 

Re: Added schema level support for publication.

[vignesh C]

On Mon, Sep 13, 2021 at 7:06 PM tanghy.f...@fujitsu.com
 wrote:
>
> On Sunday, September 12, 2021 11:13 PM vignesh C  wrote:
> >
> > Thanks for the changes, the suggested changes make the parsing code
> > simpler. I have merged the changes to the main patch. Attached v27
> > patch has the changes for the same.
> >
>
> Thanks for your new patch. I had a look at the changes related to document 
> and tried
> the patch. Here are several comments.
>
> 1.
>
> You must own the publication to use ALTER PUBLICATION.
> Adding a table to a publication additionally requires owning that table.
> +   The ADD SCHEMA and SET SCHEMA to a
> +   publication requires the invoking user to be a superuser.
> To alter the owner, you must also be a direct or indirect member of the 
> new
> owning role. The new owner must have CREATE privilege 
> on
> the database.  Also, the new owner of a FOR ALL TABLES
> publication must be a superuser.  However, a superuser can change the
> ownership of a publication regardless of these restrictions.
>
>
> ADD SCHEMA
> ->
> ADD ALL TABLES IN SCHEMA
>
> SET SCHEMA
> ->
> SET ALL TABLES IN SCHEMA

Modified

> 2.
> +  
> +   ADD some tables and schemas to the publication:
> +
> +ALTER PUBLICATION production_publication ADD TABLE users, departments, ALL 
> TABLES IN SCHEMA production;
> +
> +  
>
> ADD some tables and schemas to the publication:
> ->
> Add some tables and schemas to the publication:

Modified

> 3.
> +  
> +   Drop some schema from the publication:
> +
> +ALTER PUBLICATION production_quarterly_publication DROP ALL TABLES IN SCHEMA 
> production_july;
> +
> +  
>
> Drop some schema from the publication:
> ->
> Drop some schemas from the publication:

Modified

> 4.
> +   The catalog pg_publication_namespace contains the
> +   mapping between schemas and publications in the database.  This is a
> +   many-to-many mapping.
>
> There are two Spaces at the end of the paragraph.

I felt this is ok, as both single space and double space are used at
various places.

> 5.
> Adding a table and the schema where the table belonged to is not supported. 
> But
> it didn't report error message when I try to add them in the same statement by
> using 'ALTER PUBLICATION'.
>
> For example:
> postgres=# create publication pub;
> CREATE PUBLICATION
> postgres=# alter publication pub add  all tables in schema s1, table s1.tbl;
> ALTER PUBLICATION
> postgres=# \dRp+
>   Publication pub
>   Owner   | All tables | Inserts | Updates | Deletes | Truncates | Via root
> --++-+-+-+---+--
>  postgres | f  | t   | t   | t   | t | f
> Tables:
> "s1.tbl"
> Tables from schemas:
> "s1"
>
> It didn't check if table 's1.tbl' is member of schema 's1'.

Modified.

> 6.
> I think if I use 'ALTER PUBLICATION ... SET', both the list of tables and the
> list of all tables in schemas should be reset. The publication should only
> contain the tables and all tables in schemas which user specified. If user 
> only
> specified all tables in schema, and didn't specify tables, the tables which 
> used
> to be part of the publication should be dropped, too. But currently, if I 
> didn't
> specify tables, the list of tables wouldn't be set to empty. Thoughts?
>
> For example:
> postgres=# create publication pub for table s1.tbl;
> CREATE PUBLICATION
> postgres=# alter publication pub set all tables in schema s2;
> ALTER PUBLICATION
> postgres=# \dRp+
>   Publication pub
>   Owner   | All tables | Inserts | Updates | Deletes | Truncates | Via root
> --++-+-+-+---+--
>  postgres | f  | t   | t   | t   | t | f
> Tables:
> "s1.tbl"
> Tables from schemas:
> "s2"

Modified

I have fixed the comments in the v28 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm0OudeDeFN7bSWPro0hgKx%3D1zPgcNFWnvU_G6w3mDPX0Q%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Tue, Sep 14, 2021 at 6:31 AM houzj.f...@fujitsu.com
 wrote:
>
> From Sun, Sept 12, 2021 11:13 PM vignesh C  wrote:
> > On Fri, Sep 10, 2021 at 11:21 AM Hou Zhijie  wrote:
> > > Attach the without-flag version and add comments about the pubobj_name.
> >
> > Thanks for the changes, the suggested changes make the parsing code simpler.
> > I have merged the changes to the main patch. Attached v27 patch has the
> > changes for the same.
>
> Hi,
>
> I have some suggestions for the documents and comments of the new syntax.
>
> IMO, the document could be clearer in the following format.
> --
> Synopsis
> CREATE PUBLICATION name
> [ FOR ALL TABLES
>   | FOR publication object [, ... ] ]
> [ WITH ( publication_parameter [= value] [, ... ] ) ]
>
> where publication object is one of:
>
> TABLE [ ONLY ] table_name [ * ] [, ... ]
> ALL TABLES IN SCHEMA { schema_name | CURRENT_SCHEMA } [, ... ]
> --
>
> Attach a diff(based on v27-*) which change the doc and comments like the
> following.

Thanks for the comments and the changes, I have made a few changes and
merged it into the v28 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm0OudeDeFN7bSWPro0hgKx%3D1zPgcNFWnvU_G6w3mDPX0Q%40mail.gmail.com

Regards,
Vignesh

Re: [BUG] Unexpected action when publishing partition tables

[vignesh C]

On Tue, Sep 7, 2021 at 11:38 AM houzj.f...@fujitsu.com
 wrote:
>
> From Tues, Sep 7, 2021 12:02 PM Amit Kapila  wrote:
> > On Mon, Sep 6, 2021 at 1:49 PM houzj.f...@fujitsu.com 
> >  wrote:
> > >
> > > I can reproduce this bug.
> > >
> > > I think the reason is it didn't invalidate all the leaf partitions'
> > > relcache when add a partitioned table to the publication, so the
> > > publication info was not rebuilt.
> > >
> > > The following code only invalidate the target table:
> > > ---
> > > PublicationAddTables
> > > publication_add_relation
> > > /* Invalidate relcache so that publication info is 
> > > rebuilt. */
> > > CacheInvalidateRelcache(targetrel);
> > > ---
> > >
> > > In addition, this problem can happen in both ADD TABLE, DROP TABLE,
> > > and SET TABLE cases, so we need to invalidate the leaf partitions'
> > > recache in all these cases.
> > >
> >
> > Few comments:
> > =
> >   {
> > @@ -664,7 +673,13 @@ PublicationDropTables(Oid pubid, List *rels, bool
> > missing_ok)
> >
> >   ObjectAddressSet(obj, PublicationRelRelationId, prid);
> >   performDeletion(, DROP_CASCADE, 0);
> > +
> > + relids = GetPubPartitionOptionRelations(relids, PUBLICATION_PART_LEAF,
> > + relid);
> >   }
> > +
> > + /* Invalidate relcache so that publication info is rebuilt. */
> > + InvalidatePublicationRels(relids);
> >  }
> >
> > We already register the invalidation for the main table in
> > RemovePublicationRelById which is called via performDeletion. I think it is
> > better to perform invalidation for partitions at that place.
> > Similarly is there a reason for not doing invalidations of partitions in
> > publication_add_relation()?
>
> Thanks for the comment. I originally intended to reduce the number of invalid
> message when add/drop serval tables while each table has lots of partitions 
> which
> could exceed the MAX_RELCACHE_INVAL_MSGS. But that seems a rare case, so ,
> I changed the code as suggested.
>
> Attach new version patches which addressed the comment.

Thanks for fixing this issue. The bug gets fixed by the patch, I did
not find any issues in my testing.
I just had one minor comment:

We could clean the table at the end by calling DROP TABLE testpub_parted2:
+-- still fail, because parent's publication replicates updates
+UPDATE testpub_parted2 SET a = 2;
+ERROR:  cannot update table "testpub_parted2" because it does not
have a replica identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+ALTER PUBLICATION testpub_forparted DROP TABLE testpub_parted;
+-- works again, because update is no longer replicated
+UPDATE testpub_parted2 SET a = 2;

Regards,
Vignesh

Re: Column Filtering in Logical Replication

[vignesh C]

On Thu, Sep 16, 2021 at 8:45 AM Amit Kapila  wrote:
>
> On Wed, Sep 15, 2021 at 6:06 PM Alvaro Herrera  
> wrote:
> >
> > On 2021-Sep-15, vignesh C wrote:
> > > The patch
> > > Generic_object_type_parser_002_table_schema_publication.patch has the
> > > changes that were used to handle the parsing. Schema and Relation both
> > > are different objects, schema is of string type and relation is of
> > > RangeVar type. While parsing, schema name is parsed in string format
> > > and relation is parsed and converted to rangevar type, these objects
> > > will be then handled accordingly during post processing.
> >
> > Yeah, I think it'd be cleaner if the node type has two members, something 
> > like
> > this
> >
> > typedef struct PublicationObjSpec
> > {
> > NodeTag type;
> > PublicationObjSpecType pubobjtype;  /* type of this publication 
> > object */
> > RangeVar*rv;/* if a table */
> > String  *objname;   /* if a schema */
> > int location;   /* token location, or -1 if 
> > unknown */
> > } PublicationObjSpec;
> >
> > and only one of them is set, the other is NULL, depending on the object 
> > type.
> >
>
> I think the problem here is that with the proposed grammar we won't be
> always able to distinguish names at the gram.y stage.

This is the issue that Amit was talking about:
gram.y: error: shift/reduce conflicts: 2 found, 0 expected
gram.y: warning: shift/reduce conflict on token ',' [-Wcounterexamples]
  First example: CREATE PUBLICATION name FOR TABLE relation_expr_list
• ',' relation_expr ',' PublicationObjSpec opt_definition $end
  Shift derivation
$accept
↳ parse_toplevel
  $end
  ↳ stmtmulti
↳ toplevel_stmt
  ↳ stmt
↳ CreatePublicationStmt
  ↳ CREATE PUBLICATION name FOR pub_obj_list
   opt_definition
↳ PublicationObjSpec
',' PublicationObjSpec
  ↳ TABLE relation_expr_list
  ↳
relation_expr_list • ',' relation_expr
  Second example: CREATE PUBLICATION name FOR TABLE relation_expr_list
• ',' PublicationObjSpec opt_definition $end
  Reduce derivation
$accept
↳ parse_toplevel
$end
  ↳ stmtmulti
↳ toplevel_stmt
  ↳ stmt
↳ CreatePublicationStmt
  ↳ CREATE PUBLICATION name FOR pub_obj_list
 opt_definition
↳ pub_obj_list
  ',' PublicationObjSpec
  ↳ PublicationObjSpec
↳ TABLE relation_expr_list •
Here it is not able to distinguish if ',' is used for the next table
name or the next object.
I was able to reproduce this issue with the attached patch.

Regards,
Vignesh
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index d6fddd6efe..2a2fe03c13 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -141,14 +141,14 @@ pg_relation_is_publishable(PG_FUNCTION_ARGS)
  * Insert new publication / relation mapping.
  */
 ObjectAddress
-publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
+publication_add_relation(Oid pubid, Relation targetrel,
 		 bool if_not_exists)
 {
 	Relation	rel;
 	HeapTuple	tup;
 	Datum		values[Natts_pg_publication_rel];
 	bool		nulls[Natts_pg_publication_rel];
-	Oid			relid = RelationGetRelid(targetrel->relation);
+	Oid			relid = RelationGetRelid(targetrel);
 	Oid			prrelid;
 	Publication *pub = GetPublication(pubid);
 	ObjectAddress myself,
@@ -172,10 +172,10 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 		ereport(ERROR,
 (errcode(ERRCODE_DUPLICATE_OBJECT),
  errmsg("relation \"%s\" is already member of publication \"%s\"",
-		RelationGetRelationName(targetrel->relation), pub->name)));
+		RelationGetRelationName(targetrel), pub->name)));
 	}
 
-	check_publication_add_relation(targetrel->relation);
+	check_publication_add_relation(targetrel);
 
 	/* Form a tuple. */
 	memset(values, 0, sizeof(values));
@@ -209,7 +209,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
 	table_close(rel, RowExclusiveLock);
 
 	/* Invalidate relcache so that publication info is rebuilt. */
-	CacheInvalidateRelcache(targetrel->relation);
+	CacheInvalidateRelcache(targetrel);
 
 	return myself;
 }
diff --git a/src/ba

Re: Added schema level support for publication.

[vignesh C]

On Tue, Sep 7, 2021 at 5:10 PM Amit Kapila  wrote:
>
> On Tue, Sep 7, 2021 at 12:45 PM vignesh C  wrote:
> >
> > On Fri, Sep 3, 2021 at 4:49 PM Amit Kapila  wrote:
> > >
> >
> > > 5.
> > > If I modify the search path to remove public schema then I get the
> > > below error message:
> > > postgres=# Create publication mypub for all tables in schema 
> > > current_schema;
> > > ERROR:  no schema has been selected
> > >
> > > I think this message is not very clear. How about changing to
> > > something like "current_schema doesn't contain any valid schema"? This
> > > message is used in more than one place, so let's keep it the same at
> > > all the places if you agree to change it.
> >
> > I would prefer to use the existing messages as we have used this in a
> > few other places similarly. Thoughts?
> >
>
> Yeah, I also see the same message in code but I think here usage is a
> bit different. If you see a similar SQL statement that causes the same
> error message then can you please give an example?

I was referring to the error message in create table
postgres=# set search_path='non_existent_schema';
SET
postgres=# create table t1(c1 int);
ERROR:  no schema has been selected to create in
LINE 1: create table t1(c1 int);

If it is not very useful in the case of creating a publication, then I
can change it. Thoughts?

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Sep 3, 2021 at 4:49 PM Amit Kapila  wrote:
>
> On Thu, Sep 2, 2021 at 5:12 PM Amit Kapila  wrote:
> >
> > On Thu, Sep 2, 2021 at 11:58 AM vignesh C  wrote:
> > >
> >
> > Below are few comments on v23. If you have already addressed anything
> > in v24, then ignore it.
> >
>
> More Review comments (on latest version v24):
> ===
> 1.
> +Oidpsnspcid BKI_LOOKUP(pg_class);/* Oid of the schema */
> +} FormData_pg_publication_schema;
>
> Why in the above structure you have used pg_class instead of pg_namespace?

It should be pg_namespace, I have changed it.

> 2. GetSchemaPublicationRelations() uses two different ways to skip
> non-publishable relations in two while loops. Both are correct but I
> think it would be easier to follow if they use the same way and in
> this case I would prefer a check like if (is_publishable_class(relid,
> relForm)). The comments atop function could also be modified to :"Get
> the list of publishable relation oids for a specified schema.". I
> think it is better to write some comments on why you need to scan and
> loop twice.

Modified

> 3. The other point about GetSchemaPublicationRelations() is that I am
> afraid that it will collect duplicate relation oids in the final list
> when the partitioned table and child tables are in the same schema.

Modified it to prepare a list without duplicate relation ids.

> 4. In GetRelationPublicationActions(), the handling related to
> partitions seems to be missing for the schema. It won't be able to
> take care of child tables when they are in a different schema than the
> parent table.

Modified

> 5.
> If I modify the search path to remove public schema then I get the
> below error message:
> postgres=# Create publication mypub for all tables in schema current_schema;
> ERROR:  no schema has been selected
>
> I think this message is not very clear. How about changing to
> something like "current_schema doesn't contain any valid schema"? This
> message is used in more than one place, so let's keep it the same at
> all the places if you agree to change it.

I would prefer to use the existing messages as we have used this in a
few other places similarly. Thoughts?

> 6. How about naming ConvertPubObjSpecListToOidList() as
> ObjectsInPublicationToOids()? I see somewhat similarly named functions
> in the code like objectsInSchemaToOids, objectNamesToOids.

Modified

> 7.
> + /*
> + * Schema lock is held until the publication is created to prevent
> + * concurrent schema deletion. No need to unlock the schemas, the
> + * locks will be released automatically at the end of create
> + * publication command.
> + */
>
> In this comment no need to say create publication command as that is
> implicit, we can just write " at the end of command".

Modified

> 8. Can you please extract changes like tab-completion, dump/restore in
> separate patches? This can help to review the core (backend) part of
> the patch in more detail.

Modified

This is handled as part of v25 patch attached at [1]
[1] - 
https://www.postgresql.org/message-id/CALDaNm2SytXy2TDnzzYkXWKgNp74ssPBXrkMXEyac1qVYSRkbw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Sep 3, 2021 at 4:06 PM Amit Kapila  wrote:
>
> On Wed, Sep 1, 2021 at 12:05 PM Amit Kapila  wrote:
> >
> > On Wed, Sep 1, 2021 at 8:52 AM Greg Nancarrow  wrote:
> > >
> >
> > > I'd expect a lot of users to naturally think that "ALTER PUBLICATION
> > > pub1 DROP ALL TABLES IN SCHEMA sc1;" would drop from the publication
> > > all tables that are in schema "sc1", which is not what it is currently
> > > doing.
> > > Since the syntax was changed to specifically refer to FOR ALL TABLES
> > > IN SCHEMA rather than FOR SCHEMA, then now it's clear we're referring
> > > to tables only, when specifying "... FOR ALL TABLES in sc1, TABLE
> > > sc1.test", so IMHO it's reasonable to remove duplicates here, rather
> > > than treating these as somehow separate ways of referencing the same
> > > table.
> > >
> >
> > I see your point and if we decide to go this path then it is better to
> > give an error than silently removing duplicates.
> >
>
> Today, I have thought about this point again and it seems better to
> give an error in this case and let the user take the action rather
> than silently removing such tables to avoid any confusion.

I have handled this to throw an error. This is handled as part of v25
patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm2SytXy2TDnzzYkXWKgNp74ssPBXrkMXEyac1qVYSRkbw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Sep 3, 2021 at 3:12 PM Amit Kapila  wrote:
>
> On Mon, Aug 30, 2021 at 8:56 PM vignesh C  wrote:
> >
> > On Mon, Aug 30, 2021 at 9:10 AM houzj.f...@fujitsu.com
> >  wrote:
> > >
> >
> > > 5)
> > > +   if (list_length(pubobj->name) == 1 &&
> > > +   (strcmp(relname, "CURRENT_SCHEMA") == 0))
> > > +   ereport(ERROR,
> > > +   
> > > errcode(ERRCODE_SYNTAX_ERROR),
> > > +   errmsg("invalid relation 
> > > name at or near"),
> > > +   
> > > parser_errposition(pstate, pubobj->location));
> > >
> > > Maybe we don't need this check, because it will report an error in
> > > OpenTableList() anyway, "relation "CURRENT_SCHEMA" does not exist" , and 
> > > that
> > > message seems readable to me.
> >
> > Allowing CURRENT_SCHEMA is required to support current schema for
> > schema publications, currently I'm allowing this syntax during parsing
> > and this error is thrown for relations later, this is done to keep the
> > similar error as earlier before this feature support. I felt we can
> > keep it like this to maintain the similar error. Thoughts?
> >
>
> I find this check quite ad-hoc in the code and I am not sure if we
> need to be consistent for the exact message in this case. So, I think
> it is better to remove it.

Modified

> > > About  0003
> > > 7)
> > > The v22-0003 seems simple and can remove lots of code in patch v22-0001, 
> > > so
> > > maybe we can merge 0001 and 0003 into one patch ?
> >
> > I agree that the code becomes simpler, it reduces a lot of code. I had
> > kept it like that as the testing effort might be more and also I was
> > waiting if there was no objection for that syntax from anyone else. I
> > will wait for a few more reviews and merge it to 0001 if there are no
> > objections.
> >
>
> +1 to merge the patch as suggested by Hou-San.

Modified

This is handled as part of v25 patch attached at [1]

[1] - 
https://www.postgresql.org/message-id/CALDaNm2SytXy2TDnzzYkXWKgNp74ssPBXrkMXEyac1qVYSRkbw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Sep 6, 2021 at 6:56 AM houzj.f...@fujitsu.com
 wrote:
>
> From Thur, Sep 2, 2021 7:42 PM Amit Kapila  wrote:
> > On Thu, Sep 2, 2021 at 11:58 AM vignesh C  wrote:
> > >
> >
> > Below are few comments on v23. If you have already addressed anything in 
> > v24,
> > then ignore it.
> >
> > 1. The commit message says: A new system table "pg_publication_schema"
> > has been added, to maintain the schemas that the user wants to publish
> > through the publication.". The alternative name for this system table could 
> > be
> > "pg_publication_namespace". The reason why this alternative comes to my
> > mind is that the current system table to store schema information is named
> > pg_namespace. So shouldn't we be consistent here?
> > What do others think about this?
>
> Since the new system table refer to pg_namespace, so, personally, I am +1 for
> "pg_publication_namespace".

I have changed it as part of v25 patch attached at [1]
[1] - 
https://www.postgresql.org/message-id/CALDaNm2SytXy2TDnzzYkXWKgNp74ssPBXrkMXEyac1qVYSRkbw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Sep 6, 2021 at 6:56 AM houzj.f...@fujitsu.com
 wrote:
>
> From Thur, Sep 2, 2021 2:33 PM vignesh C  wrote:
> > On Wed, Sep 1, 2021 at 6:58 AM houzj.f...@fujitsu.com 
> >  wrote:
> > >
> > > Here are some other comments for v23-000x patches.
> > > 3)
> > >
> > > + .description =
> > "PUBLICATION SCHEMA",
> > > + .section =
> > SECTION_POST_DATA,
> > > + .createStmt
> > > + = query->data));
> > >
> > > Is it better to use something like 'PUBLICATION TABLES IN SCHEMA' to
> > > describe the schema level table publication ? Because there could be
> > > some other type publication such as 'ALL SEQUENCES IN SCHEMA' in the
> > > future, it will be better to make it clear that we only publish table in 
> > > schema in
> > this patch.
> >
> > Modified
>
> Thanks for updating the patch.
>
> I think we might also need to mention the publication object 'table' in the
> following types:
>
> 1)
> +   /* OCLASS_PUBLICATION_SCHEMA */
> +   {
> +   "publication schema", OBJECT_PUBLICATION_SCHEMA
> +   },
>
> 2)
> +   PUBLICATIONOBJ_SCHEMA,  /* Schema type */
> +   PUBLICATIONOBJ_UNKNOWN  /* Unknown type */
> +} PublicationObjSpecType;
>
> 3)
> +   DO_PUBLICATION_SCHEMA,
>
> I think it might be to change the typename like XX_REL_IN_SCHEMA,
> and adjust the comments.

Thanks for the comments, this is handled in the v26 patch attached at [1]
[1] - 
https://www.postgresql.org/message-id/CALDaNm3EwAVma8n4YpV1%2BQWiccuVPxpqNfbbrUU3s3XTHcTXew%40mail.gmail.com

Regards,
Vignesh

Re: Column Filtering in Logical Replication

[vignesh C]

On Thu, Sep 16, 2021 at 7:20 PM Alvaro Herrera  wrote:
>
> On 2021-Sep-16, vignesh C wrote:
>
> > diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> > index e3068a374e..c50bb570ea 100644
> > --- a/src/backend/parser/gram.y
> > +++ b/src/backend/parser/gram.y
>
> Yeah, on a quick glance this looks all wrong.  Your PublicationObjSpec
> production should return a node with tag PublicationObjSpec, and
> pubobj_expr should not exist at all -- that stuff is just making it all
> more confusing.
>
> I think it'd be something like this:
>
> PublicationObjSpec:
> ALL TABLES
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_ALL_TABLES;
> $$->location = @1;
> }
> | TABLE qualified_name
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_TABLE;
> $$->pubobj = $2;
> $$->location = @1;
> }
> | ALL TABLES IN_P SCHEMA name
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_ALL_TABLES_IN_SCHEMA;
> $$->pubobj = makeRangeVar( 
> ... $5 ... );
> $$->location = @1;
> }
> | qualified_name
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_CONTINUATION;
> $$->pubobj = $1;
> $$->location = @1;
> };
>
> You need a single object name under TABLE, not a list -- this was Tom's
> point about needing post-processing to determine how to assign a type to
> a object that's what I named PUBLICATIONOBJ_CONTINUATION here.

In the above, we will not be able to use qualified_name, as
qualified_name will not support the following syntaxes:
create publication pub1 for table t1 *;
create publication pub1 for table ONLY t1 *;
create publication pub1 for table ONLY (t1);

To solve this problem we can change qualified_name to relation_expr
but the problem with doing that is that the user will be able to
provide the following syntaxes:
create publication pub1 for all tables in schema sch1 *;
create publication pub1 for all tables in schema ONLY sch1 *;
create publication pub1 for all tables in schema ONLY (sch1);

To handle this we will need some special flag which will differentiate
these and throw errors at post processing time. We need to define an
expression similar to relation_expr say pub_expr which handles all
variants of qualified_name and then use a special flag so that we can
throw an error if somebody uses the above type of syntax for schema
names. And then if we have to distinguish between schema name and
relation name variant, then we need few other things.

We proposed the below solution which handles all these problems and
also used Node type which need not store schemaname in RangeVar type:
pubobj_expr:
pubobj_name
{
/* inheritance query, implicitly */
$$ = makeNode(PublicationObjSpec);
$$->object = $1;
}
| extended_relation_expr
{
$$ = makeNode(PublicationObjSpec);
$$->object = (Node *)$1;
}
| CURRENT_SCHEMA
{
$$ = makeNode(PublicationObjSpec);
$$->object = (Node
*)makeString("CURRENT_SCHEMA");
}
;
/* This can be either a schema or relation n

Re: Added schema level support for publication.

[vignesh C]

On Thu, Sep 16, 2021 at 9:54 AM Amit Kapila  wrote:
>
> On Wed, Sep 15, 2021 at 12:30 PM Amit Kapila  wrote:
> >
> > On Tue, Sep 14, 2021 at 2:08 PM vignesh C  wrote:
> > >
> > > I have handled this in the patch attached.
> > >
> >
> > 4.
> > AlterPublicationSchemas()
> > {
> > ..
> > + /*
> > + * If the table option was not specified remove the existing tables
> > + * from the publication.
> > + */
> > + if (!tables)
> > + {
> > + rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
> > + PublicationDropTables(pubform->oid, rels, false, true);
> > + }
> > +
> > + /* Identify which schemas should be dropped */
> > + delschemas = list_difference_oid(oldschemaids, schemaidlist);
> > +
> > + /* And drop them */
> > + PublicationDropSchemas(pubform->oid, delschemas, true);
> >
> > Here, you have neither locked tables to be dropped nor schemas. I
> > think both need to be locked as we do for tables in similar code in
> > AlterPublicationTables(). Can you please test via debugger what
> > happens if we try to drop without taking lock here and concurrently
> > try to drop the actual object? It should give some error. If we decide
> > to lock here then we should be able to pass the list of relations to
> > PublicationDropTables() instead of Oids which would then obviate the
> > need for any change to that function.
> >
> > Similarly don't we need to lock schemas before dropping them in
> > AlterPublicationTables()?
> >
>
> I think there is one more similar locking problem.
> AlterPublicationSchemas()
> {
> ..
> + if (stmt->action == DEFELEM_ADD)
> + {
> + List *rels;
> +
> + rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
> + RelSchemaIsMemberOfSchemaList(rels, schemaidlist, true);
> ...
> ...
> }
>
> Here, we don't have a lock on the relation. So, what if the relation
> is concurrently dropped after you get the rel list by
> GetPublicationRelations?

This works fine without locking even after concurrent drop, I felt
this works because of MVCC.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Thu, Sep 16, 2021 at 8:59 AM houzj.f...@fujitsu.com
 wrote:
>
> On Tuesday, September 14, 2021 4:39 PM vignesh C  wrote:
> >
> > I have handled this in the patch attached.
>
> Thanks for updating the patch.
> Here are some comments.
>
> 1)
> +static void
> +AlterPublicationSchemas(AlterPublicationStmt *stmt, Relation rel,
> ...
> +   /*
> +* If the table option was not specified remove the existing 
> tables
> +* from the publication.
> +*/
> +   if (!tables)
> +   {
> +   rels = GetPublicationRelations(pubform->oid, 
> PUBLICATION_PART_ROOT);
> +   PublicationDropTables(pubform->oid, rels, false, 
> true);
> +   }
>
>
> It seems not natural to drop tables in AlterPublication*Schemas*,
> I think we'd better do it in AlterPublicationTables.

I felt keeping the current way keeps it better to avoid additional
checks. Thoughts?

> 2)
>  static void
>  AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
>  ...
> +   /*
> +* If ALL TABLES IN SCHEMA option was not specified 
> remove the
> +* existing schemas from the publication.
> +*/
> +   List *pubschemas = GetPublicationSchemas(pubid);
> +   PublicationDropSchemas(pubform->oid, pubschemas, 
> false);
>
> Same as 1), Is it better to modify the schema list in AlterPublicationSchemas 
> ?

This is similar to above.

> 3)
>  static void
>  AlterPublicationTables(AlterPublicationStmt *stmt, Relation rel,
> ...
> /* check if the relation is member of the schema list 
> specified */
> RelSchemaIsMemberOfSchemaList(rels, schemaidlist, false);
>
> IIRC, The check here is to check the specified tables and schemas in the
> command. Personally, this seems a common operation which can be placed in
> function AlterPublication(). If we move this check to AlterPublication() and 
> if
> comment 1) and 2) makes sense to you, then we don't need the new function
> parameters in AlterPublicationTables() and AlterPublicationSchemas().

I felt we can keep the checks as is currently, else we will have to
extra checks outside and addition calls for conversion from oid to
Relation like:
  if (stmt->options)
AlterPublicationOptions(pstate, stmt, rel, tup);
  else
  {
if (relations)
{
  if (stmt->action != DEFELEM_DROP)
  {
List *rels = OpenTableList(relations);

/* check if relation is member of the schema list specified */
RelSchemaIsMemberOfSchemaList(rels, schemaidlist, false);
CloseTableList(rels);
  }

  AlterPublicationTables(stmt, rel, tup, relations,
   list_length(schemaidlist));
}
if (schemaidlist)
  AlterPublicationSchemas(stmt, rel, tup, schemaidlist,
  list_length(relations));
  }

Thoughts?

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Thu, Sep 16, 2021 at 11:24 AM Amit Kapila  wrote:
>
> On Wed, Sep 15, 2021 at 4:45 PM Greg Nancarrow  wrote:
> >
> > On Tue, Sep 14, 2021 at 6:38 PM vignesh C  wrote:
> > >
> > > I have handled this in the patch attached.
> > >
> >
> > Regarding the following function in the v28-0002 patch:
> >
> > +/*
> > + * Check if the relation schema is member of the schema list.
> > + */
> > +static void
> > +RelSchemaIsMemberOfSchemaList(List *rels, List *schemaidlist, bool 
> > schemacheck)
> >
> > I think this function is not well named or commented, and I don't like
> > how the "schemacheck" bool parameter determines the type of objects in
> > the "rels" list.
> >
>
> I think after fixing the comments in my previous email, the rels list
> will become the same for this function but surely the extra parameter
> is required for giving object-specific errors.
>
> > I would suggest you simply split this function into two separate
> > functions, corresponding to each of the blocks of the "if-else" within
> > the for-loop of the existing RelSchemaIsMemberOfSchemaList function.
> > The "Is" part of the existing "RelSchemaIsMemberOfSchemaList" function
> > name implies a boolean return value, so seems misleading.
> > So I think the names of the two functions that I am suggesting should
> > be "CheckNotAlreadyInPublication" or something similar.
> >
>
> I think if we write individual functions then we need to add new
> functions as and when we add new object types like sequences. The
> other idea could be to keep a single function like now say
> CheckObjSchemaNotAlreadyInPublication and instead of the bool
> parameter as the patch has now, we can keep an enum parameter
> "add_obj_type" for 'rel', 'schema', 'sequence'. We can either use
> exiting enum PublicationObjSpecType or define a new one for the same.

Modified the function name and changed the parameter to
PublicationObjSpecType. The changes are present at the v29 patch
posted at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm1Wb%3D_HGd85wp2WM%2BfLc-8PSJ824TOZEJ6nDz3akWTidw%40mail.gmail.com

Regards,
Vignesh

Re: Printing backtrace of postgres processes

[vignesh C]

On Wed, May 12, 2021 at 2:27 AM Robert Haas  wrote:
>
> On Thu, May 6, 2021 at 3:31 PM Tom Lane  wrote:
> > Andres Freund  writes:
> > > On 2021-05-06 14:56:09 -0400, Tom Lane wrote:
> > >> If we think it's worth having a predefined role for, OK.  However,
> > >> I don't like the future I see us heading towards where there are
> > >> hundreds of random predefined roles.  Is there an existing role
> > >> that it'd be reasonable to attach this ability to?
> >
> > > It does seem like it'd be good to group it in with something
> > > else. There's nothing fitting 100% though.
> >
> > I'd probably vote for pg_read_all_data, considering that much of
> > the concern about this has to do with the possibility of exposure
> > of sensitive data.  I'm not quite sure what the security expectations
> > are for pg_monitor.
>
> Maybe we should have a role that is specifically for server debugging
> type things. This kind of overlaps with Mark Dilger's proposal to try
> to allow SET for security-sensitive GUCs to be delegated via
> predefined roles. The exact way to divide that up is open to question,
> but it wouldn't seem crazy to me if the same role controlled the
> ability to do this plus the ability to set the GUCs
> backtrace_functions, debug_invalidate_system_caches_always,
> wal_consistency_checking, and maybe a few other things.

+1 for the idea of having a new role for this. Currently I have
implemented this feature to be supported only for the superuser. If we
are ok with having a new role to handle debugging features, I will
make a 002 patch to handle this.

Regards,
Vignesh

Re: Add option --drop-cascade for pg_dump/restore

[vignesh C]

On Fri, Jul 2, 2021 at 12:11 PM Haotian Wu  wrote:
>
> Hi,
>
> I agree that —drop-cascade does not make sense for pg_dumpall, so I removed 
> them.
>
> > are we expecting more things to appear after the semi-colon?
>
> No, I was just trying to “reuse” original statement as much as possible. 
> Append “\n” manually should also do the job, and I’ve updated the patch as 
> you suggests.

1) This change is not required as it is not supported for pg_dumpall
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -289,6 +289,16 @@ PostgreSQL documentation
   
  

+ 
+  --drop-cascade
+  
+   
+Use CASCADE to drop database objects.
+This option is not valid unless --clean is
also specified.
+   
+  
+ 
+

2) I felt pg_dump will include the cascade option for plain format and
pg_restore will include the cascade option from pg_restore for other
formats. If my understanding is correct, should we document this?

3) This change is not required

destroyPQExpBuffer(ftStmt);
pg_free(dropStmtOrig);
}
+
}

4) Is it possible to add a few tests for this?

Regards,
Vignesh

Re: Enhanced error message to include hint messages for redundant options error

[vignesh C]

On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed  wrote:
>
> On Mon, 12 Jul 2021 at 17:39, vignesh C  wrote:
> >
> > Thanks for your comments, I have made the changes for the same in the
> > V10 patch attached.
> > Thoughts?
> >
>
> I'm still not happy about changing so many error messages.
>
> Some of the changes might be OK, but aren't strictly necessary. For example:
>
>  COPY x from stdin (force_not_null (a), force_not_null (b));
> -ERROR:  conflicting or redundant options
> +ERROR:  option "force_not_null" specified more than once
>  LINE 1: COPY x from stdin (force_not_null (a), force_not_null (b));
> ^
>
> I actually prefer the original primary error message, for consistency
> with other similar cases, and I think the error position indicator is
> sufficient to identify the problem. If it were to include the
> "specified more than once" text, I would put that in DETAIL.
>
> Other changes are wrong though. For example:
>
>  COPY x from stdin (format CSV, FORMAT CSV);
> -ERROR:  conflicting or redundant options
> +ERROR:  redundant options specified
>  LINE 1: COPY x from stdin (format CSV, FORMAT CSV);
> ^
>
> The problem here is that the code that throws this error throws the
> same error if the second format is different, which would make it a
> conflicting option, not a redundant one. And I don't think we should
> add more code to test whether it's conflicting or redundant, so again,
> I think we should just keep the original error message.
>
> Similarly, this error is wrong:
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE 
> IMMUTABLE;
> ERROR:  redundant options specified
> LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STABLE IMMUTABLE;
>  ^
>
> And even this error:
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
> ERROR:  redundant options specified
> LINE 1: ... FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT STRICT;
> ^
>
> which looks OK, is actually problematic because the same code also
> handles the alternate syntax, which leads to this (which is now wrong
> because it's conflicting not redundant):
>
> CREATE OR REPLACE FUNCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT
> CALLED ON NULL INPUT;
> ERROR:  redundant options specified
> LINE 1: ...NCTION foo() RETURNS int AS $$ SELECT 1 $$ STRICT CALLED ON ...
>  ^
>
> The problem is it's actually quite hard to decide in each case whether
> the option is redundant or conflicting. Sometimes, it might look
> obvious in the code, but actually be much more subtle, due to an
> earlier transformation of the grammar. Likewise redundant doesn't
> necessarily mean literally specified more than once.
>
> Also, most of these don't have regression test cases, and I'm very
> reluctant to change them without proper testing, and that would make
> the patch much bigger. To me, this patch is already attempting to
> change too much in one go, which is causing problems.
>
> So I suggest a more incremental approach, starting by keeping the
> original error message, but improving it where possible with the error
> position. Then maybe move on to look at specific cases that can be
> further improved with additional detail (keeping the same primary
> error message, for consistency).

I'm fine with this approach as we do not have tests to cover all the
error conditions, also I'm not sure if it is worth adding tests for
all the error conditions and as the patch changes a large number of
error conditions, an incremental approach is better.

> Here is an updated version, following that approach. It does the following:
>
> 1). Keeps the same primary error message ("conflicting or redundant
> options") in all cases.
>
> 2). Uses errorConflictingDefElem() to throw it, to ensure consistency
> and reduce the executable size.
>
> 3). Includes your enhancements to make the ParseState available in
> more places, so that the error position indicator is shown to indicate
> the cause of the error.
>
> IMO, this makes for a much safer incremental change, that is more committable.
>
> As it turns out, there are 110 cases of this error that now use
> errorConflictingDefElem(), and of those, just 10 (in 3 functions)
> don't have a ParseState readily available to them:
>
> - ATExecSetIdentity()
> - parse_output_parameters() x5
> - parseCreateReplSlotOptions() x4
>
> It might be possible to improve those (and

Re: DROP INDEX CONCURRENTLY on partitioned index

[vignesh C]

On Wed, Oct 28, 2020 at 6:14 AM Justin Pryzby  wrote:
>
> Forking this thread, since the existing CFs have been closed.
> https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
>
> On Mon, Sep 14, 2020 at 09:31:03AM -0500, Justin Pryzby wrote:
> > On Sat, Sep 12, 2020 at 10:35:34AM +0900, Michael Paquier wrote:
> > > On Fri, Sep 11, 2020 at 07:13:01PM -0500, Justin Pryzby wrote:
> > > > On Tue, Sep 08, 2020 at 01:31:05PM +0900, Michael Paquier wrote:
> > > >> - CIC on partitioned relations.  (Should we also care about DROP INDEX
> > > >> CONCURRENTLY as well?)
> > > >
> > > > Do you have any idea what you think that should look like for DROP INDEX
> > > > CONCURRENTLY ?
> > >
> > > Making the maintenance of the partition tree consistent to the user is
> > > the critical part here, so my guess on this matter is:
> > > 1) Remove each index from the partition tree and mark the indexes as
> > > invalid in the same transaction.  This makes sure that after commit no
> > > indexes would get used for scans, and the partition dependency tree
> > > pis completely removed with the parent table.  That's close to what we
> > > do in index_concurrently_swap() except that we just want to remove the
> > > dependencies with the partitions, and not just swap them of course.
> > > 2) Switch each index to INDEX_DROP_SET_DEAD, one per transaction
> > > should be fine as that prevents inserts.
> > > 3) Finish the index drop.
> > >
> > > Step 2) and 3) could be completely done for each index as part of
> > > index_drop().  The tricky part is to integrate 1) cleanly within the
> > > existing dependency machinery while still knowing about the list of
> > > partitions that can be removed.  I think that this part is not that
> > > straight-forward, but perhaps we could just make this logic part of
> > > RemoveRelations() when listing all the objects to remove.
> >
> > Thanks.
> >
> > I see three implementation ideas..
> >
> > 1. I think your way has an issue that the dependencies are lost.  If 
> > there's an
> > interruption, the user is maybe left with hundreds or thousands of detached
> > indexes to clean up.  This is strange since there's actually no detach 
> > command
> > for indexes (but they're implicitly "attached" when a matching parent index 
> > is
> > created).  A 2nd issue is that DETACH currently requires an exclusive lock 
> > (but
> > see Alvaro's WIP patch).
>
> I think this is a critical problem with this approach.  It's not ok if a
> failure leaves behind N partition indexes not attached to any parent.
> They may have pretty different names, which is a mess to clean up.
>
> > 2. Maybe the easiest way is to mark all indexes invalid and then drop all
> > partitions (concurrently) and then the partitioned parent.  If interrupted,
> > this would leave a parent index marked "invalid", and some child tables 
> > with no
> > indexes.  I think this may be "ok".  The same thing is possible if a 
> > concurrent
> > build is interrupted, right ?
>
> I think adding the "invalid" mark in the simple/naive way isn't enough - it 
> has
> to do everything DROP INDEX CONCURRENTLY does (of course).
>
> > 3. I have a patch which changes index_drop() to "expand" a partitioned 
> > index into
> > its list of children.  Each of these becomes a List:
> > | indexId, heapId, userIndexRelation, userHeapRelation, heaplocktag, 
> > heaprelid, indexrelid
> > The same process is followed as for a single index, but handling all 
> > partitions
> > at once in two transactions total.  Arguably, this is bad since that 
> > function
> > currently takes a single Oid but would now ends up operating on a list of 
> > indexes.
>
> This is what's implemented in the attached.  It's very possible I've missed
> opportunities for better simplification/integration.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: [PATCH] New default role allowing to change per-role/database settings

[vignesh C]

On Wed, Apr 7, 2021 at 5:23 PM Michael Banck  wrote:
>
> Hi,
>
> Am Dienstag, den 06.04.2021, 15:37 +0200 schrieb Michael Banck:
> > Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost:
> > > Should drop the 'DEFAULT_' to match the others since the rename to
> > > 'predefined' roles went in.
> >
> > Right, will send a rebased patch ASAP.
>
> Here's a rebased version that also removes the ALTER DATABASE SET
> capability from this new predefined role and adds some documentation.
>

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Make Append Cost aware of some run time partition prune case

[vignesh C]

On Thu, Mar 4, 2021 at 9:51 AM Andy Fan  wrote:
>
>
>>
>> I have implemented a new one, which only handles 1 level of partitioned 
>> table, and
>> only 1 partition key.  and only handle the eq operators like partkey = $1 / 
>> partkey in ($1, $2)
>> / parkey = $1 or partkey = $2;  The patch works well in my user case.  I can 
>> send
>> one on the latest master shortly, and hope it is helpful for you as well.
>>
>
> Hi:
>
> Here is the new patch for this topic,  which has proved works in my limited 
> user
> case (apply the similar logic on pg11), and it is incomplete since more cases
> should be covered but not.  Uploading this patch now is just for discussing 
> and
> testing.
>
> Design principle:
>
> 1). the cost of AppendPath should be reduced for either init partition prune 
> or run
> time partition prune. All of the startup_cost, total_cost, rows should be
> adjusted. As for the startup_cost, if we should adjust it carefully, or else 
> we can
> get the run_time_cost less than 0.
>
> 2). When we merge the subpath from sub-partitioned rel via
> accumulate_append_subpath, currently we just merge the subpaths and discard 
> the
> cost/rows in AppendPath.  After this feature is involved, we may consider to 
> use
> the AppendPath's cost as well during this stage.
>
> 3). When join is involved, AppendPath is not enough since the estimated rows 
> for
> a join relation cares about rel1->rows, rel2->rows only, the Path.rows is not
> cared. so we need to adjust rel->rows as well.  and only for the init
> partition prune case.  (appendrel->rows for planning time partition prune is
> handled already).
>
> The biggest problem of the above is I don't know how to adjust the cost for
> Parallel Append Path. Currently I just ignore it, which would cause some query
> should use Parallel Append Path but not.
>
> Something I don't want to handle really unless we can address its value.
> 1. Operators like  >, <. Between and.
> 2. the uncompleted part key appeared in prunequals. Like we have partition 
> key (a,
>b). But use just use A = 1 as restrictinfo.
>
> The main reason I don't want to handle them are 1). it would be uncommon.
> b). It introduces extra complexity. c). at last, we can't estimate it well 
> like partkey > $1,
> what would be a prune ratio for ).
>
> Something I don't handle so far are: 1). accumulate_append_subpath
> stuff. 2). MergeAppend.  3). Multi Partition key.
>

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: storing an explicit nonce

[vignesh C]

On Sat, Jun 26, 2021 at 2:52 AM Bruce Momjian  wrote:
>
> On Wed, May 26, 2021 at 05:02:01PM -0400, Bruce Momjian wrote:
> > For these reasons, if we decide to go in the direction of using a
> > non-LSN nonce, I no longer plan to continue working on this feature. I
> > would rather work on things that have a more positive impact.  Maybe a
> > non-LSN nonce is a better long-term plan, but there are too many
> > unknowns and complexity for me to feel comfortable with it.
>
> As stated above, I have no plans to continue working on this feature.  I
> am attaching my final patches here in case anyone wants to make use of
> them;  it passes check-world and all my private tests.  I have removed
> my patches from the feature wiki page:
>
> https://wiki.postgresql.org/wiki/Transparent_Data_Encryption
>
> and replaced it with a link to this email.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Reduce the number of special cases to build contrib modules on windows

[vignesh C]

On Mon, Apr 19, 2021 at 5:18 PM David Rowley  wrote:
>
> On Wed, 3 Mar 2021 at 22:37, David Rowley  wrote:
> > I've attached a rebased patch.
>
> I've rebased this again.
>
> I also moved away from using hash tables for storing references and
> libraries.  I was having some problems getting psql to compile due to
> the order of the dependencies being reversed due to the order being at
> the mercy of Perl's hash function. There's mention of this in
> Makefile.global.in:
>
> # libpq_pgport is for use by client executables (not libraries) that use 
> libpq.
> # We force clients to pull symbols from the non-shared libraries libpgport
> # and libpgcommon rather than pulling some libpgport symbols from libpq just
> # because libpq uses those functions too.  This makes applications less
> # dependent on changes in libpq's usage of pgport (on platforms where we
> # don't have symbol export control for libpq).  To do this we link to
> # pgport before libpq.  This does cause duplicate -lpgport's to appear
> # on client link lines, since that also appears in $(LIBS).
> # libpq_pgport_shlib is the same idea, but for use in client shared libraries.
>
> I switched these back to arrays but added an additional check to only
> add new items to the array if we don't already have an element with
> the same value.
>
> I've attached the diffs in the *.vcxproj files between patched and unpatched.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Release SPI plans for referential integrity with DISCARD ALL

[vignesh C]

On Wed, Mar 10, 2021 at 1:49 PM yuzuko  wrote:
>
> Hello,
>
> I thought about this suggestion again.
>
> Amit's patch suggested in the thread [1] can eliminate SPI plans from
> INSERT/UPDATE triggers, so our memory pressure issue would be solved.
> But as far as I can see that thread, Amit's patch doesn't cover DELETE case.
> It is not a common case, but there is a risk of pressing memory
> capacity extremely.
> Considering that, this suggestion might still have good value as Corey
> said before.
>
> I improved a patch according to Peter's following comment :
> > but I think the
> > solution of dropping all cached plans as part of DISCARD ALL seems a bit
> > too extreme of a solution.  In the context of connection pooling,
> > getting a new session with pre-cached plans seems like a good thing, and
> > this change could potentially have a performance impact without a
> > practical way to opt out.
>
> In a new patch, I separated discarding SPI Plan caches for RI from DISCARD ALL
> and added a new option "RIPLANS" to DISCARD command to do that.  Also a new
> function, ResetRIPlanCache() which clears SPI plan caches is called by
> DISCARD ALL
> or DISCARD RIPLANS.  The amount of modification is small and this option can 
> be
> removed instantly when it is no longer needed, so I think we can use
> this patch as
> a provisional solution.
>
> Any thoughts?

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: [PATCH] Allow multiple recursive self-references

[vignesh C]

On Wed, Mar 31, 2021 at 7:28 PM Denis Hirn  wrote:
>
> Sorry, I didn't append the patch properly.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[vignesh C]

On Tue, Mar 30, 2021 at 2:14 AM Mark Rofail  wrote:
>
> Hey Alvaro
>
>> Yes, we should do that.
>
> I have attached v12 with more tests in “ src/test/regress/sql/gin.sql”
>
> Changelog:
> - v12 (compatible with current master 2021/03/29, commit 
> 6d7a6feac48b1970c4cd127ee65d4c487acbb5e9)
> * add more tests to “ src/test/regress/sql/gin.sql”
> * merge Andreas' edits to the GIN patch
>
> Also, @Andreas Karlsson, waiting for your edits to "pg_constraint"

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: VACUUM (DISABLE_PAGE_SKIPPING on)

[vignesh C]

On Thu, Apr 8, 2021 at 11:40 PM Simon Riggs  wrote:
>
> On Thu, 8 Apr 2021 at 18:15, Alvaro Herrera  wrote:
> >
> > On 2021-Apr-08, Simon Riggs wrote:
> >
> > > On Thu, 8 Apr 2021 at 16:58, David Steele  wrote:
> >
> > > > It's not clear to me which patch is which, so perhaps move one CF entry
> > > > to next CF and clarify which patch is current?
> > >
> > > Entry: Maximize page freezing
> > > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov.
> > > one_freeze_then_max_freeze.v7.patch
> >
> > Oh dear, was this waiting on me?  It was not in my to-do, so I didn't
> > pay close attention.
>
> No problem, if I felt the idea deserved priority attention I would
> have pinged you.
>
> I think I'll open a new thread later to allow it to be discussed
> without confusion.

The patch no longer applies on the head, please post a rebased patch.

Regards,
Vignesh

Re: Failed transaction statistics to measure the logical replication progress

[vignesh C]

On Thu, Jul 8, 2021 at 12:25 PM osumi.takami...@fujitsu.com
 wrote:
>
> Hello, hackers
>
>
> When the current HEAD fails during logical decoding, the failure
> increments txns count in pg_stat_replication_slots - [1] and adds
> the transaction size to the sum of bytes in the same repeatedly
> on the publisher, until the problem is solved.
> One of the good examples is duplication error on the subscriber side
> and this applies to both streaming and spill cases as well.
>
> This update prevents users from grasping the exact number and size of
> successful and unsuccessful transactions. Accordingly, we need to
> have new columns of failed transactions that will work to differentiate
> both of them for all types, which means spill, streaming and normal
> transactions. This will help users to measure the exact status of
> logical replication.
>
> Attached file is the POC patch for this.
> Current design is to save failed stats data in the ReplicationSlot struct.
> This is because after the error, I'm not able to access the ReorderBuffer 
> object.
> Thus, I chose the object where I can interact with at the 
> ReplicationSlotRelease timing.
>
> Below is one example that I can get on the publisher,
> after the duplication error on the subscriber caused by insert is solved.
>
> postgres=# select * from pg_stat_replication_slots;
> -[ RECORD 1 ]---+--
> slot_name   | mysub
> spill_txns  | 0
> spill_count | 0
> spill_bytes | 0
> failed_spill_txns   | 0
> failed_spill_bytes  | 0
> stream_txns | 0
> stream_count| 0
> stream_bytes| 0
> failed_stream_txns  | 0
> failed_stream_bytes | 0
> total_txns  | 4
> total_bytes | 528
> failed_total_txns   | 3
> failed_total_bytes  | 396
> stats_reset |
>
>
> Any ideas and comments are welcome.

+1 for having logical replication failed statistics. Currently if
there is any transaction failure in the subscriber after sending the
decoded data to the subscriber like constraint violation, object not
exist, the statistics will include the failed decoded transaction info
and there is no way to identify the actual successful transaction
data. This patch will help in measuring the actual decoded transaction
data.

Regards,
Vignesh

Re: POC: Cleaning up orphaned files using undo logs

[vignesh C]

On Wed, Jun 30, 2021 at 11:10 PM Antonin Houska  wrote:
>
> Antonin Houska  wrote:
>
> > tsunakawa.ta...@fujitsu.com  wrote:
> >
> > > I'm crawling like a snail to read the patch set.  Below are my first set 
> > > of review comments, which are all minor.
> >
> > Thanks.
>
> I've added the patch to the upcoming CF [1], so it possibly gets more review
> and makes some progress. I've marked myself as the author so it's clear who
> will try to respond to the reviews. It's clear that other people did much more
> work on the feature than I did so far - they are welcome to add themselves to
> the author list.
>
> [1] https://commitfest.postgresql.org/33/3228/
>

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Enhanced error message to include hint messages for redundant options error

[vignesh C]

On Thu, Jul 15, 2021 at 1:40 PM Dean Rasheed  wrote:
>
> On Tue, 13 Jul 2021 at 15:30, vignesh C  wrote:
> >
> > On Tue, Jul 13, 2021 at 4:25 PM Dean Rasheed  
> > wrote:
> > >
> > > As it stands, the improvements from (3) seem quite worthwhile. Also,
> > > the patch saves a couple of hundred lines of code, and for me an
> > > optimised executable is around 30 kB smaller, which is more than I
> > > expected.
> >
> > Agreed, it can be handled as part of the 2nd patch. The changes you
> > made apply neatly and the test passes.
>
> Pushed.
>
> I noticed that it's actually safe to call parser_errposition() with a
> null ParseState, so I simplified the ereport() code to just call it
> unconditionally. Also, I decided to not bother using the new function
> in cases with a null ParseState anyway since it doesn't provide any
> meaningful benefit in those cases, and those are the cases most likely
> to targeted next, so it didn't seem sensible to change that code, only
> for it to be changed again later.
>
> Probably the thing to think about next is the few remaining cases that
> throw this error directly and don't have any errdetail or errhint to
> help the user identify the offending option. My preference remains to
> leave the primary error text unchanged, but just add some suitable
> errdetail. Also, it's probably not worth adding a new function for
> those remaining errors, since there are only a handful of them.

Thanks for pushing this patch. I have changed the commitfest status to
"waiting for author" till 0002 patch is posted.

Regards,
Vignesh

Re: psql - add SHOW_ALL_RESULTS option

[vignesh C]

On Sat, Jun 12, 2021 at 3:11 PM Fabien COELHO  wrote:
>
>
> Hello Peter,
>
> >> My overly naive trust in non regression test to catch any issues has been
> >> largely proven wrong. Three key features do not have a single tests. Sigh.
> >>
> >> I'll have some time to look at it over next week-end, but not before.
> >
> > I have reverted the patch and moved the commit fest entry to CF 2021-07.
>
> Attached a v7 which fixes known issues.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Split xlog.c

[vignesh C]

On Tue, Jun 22, 2021 at 2:37 AM Heikki Linnakangas  wrote:
>
> On 17/06/2021 02:00, Andres Freund wrote:
> > On 2021-06-16 16:30:45 +0300, Heikki Linnakangas wrote:
> >> That's a fairly clean split.  StartupXLOG() stays in xlog.c, but much of 
> >> the
> >> code from it has been moved to new functions InitWalRecovery(),
> >> PerformWalRecovery() and EndWalRecovery(). The general idea is that xlog.c 
> >> is
> >> still responsible for orchestrating the servers startup, but xlogrecovery.c
> >> is responsible for figuring out whether WAL recovery is needed, performing
> >> it, and deciding when it can stop.
> >
> > For some reason "recovery" bothers me a tiny bit, even though it's obviously
> > already in use. Using "apply", or "replay" seems more descriptive to me, but
> > whatever.
>
> I think of "recovery" as a broader term than applying or replaying.
> Replaying the WAL records is one part of recovery. But yeah, the
> difference is not well-defined and we tend to use those terms
> interchangeably.
>
> >> There's surely more refactoring we could do. xlog.c has a lot of global
> >> variables, with similar names but slightly different meanings for example.
> >> (Quick: what's the difference between InRedo, InRecovery, 
> >> InArchiveRecovery,
> >> and RecoveryInProgress()? I have to go check the code every time to remind
> >> myself). But this patch tries to just move source code around for clarity.
> >
> > Agreed, it's quite chaotic. I think a good initial step to clean up that 
> > mess
> > would be to just collect the relevant variables into one or two structs.
>
> Not a bad idea.
>
> >> There are small changes in the order that some of things are done in
> >> StartupXLOG(), for readability. I tried to be careful and check that the
> >> changes are safe, but a second pair of eyes would be appreciated on that.
> >
> > I think it might be worth trying to break this into a bit more incremental
> > changes - it's a huge commit and mixing code movement with code changes 
> > makes
> > it really hard to review the non-movement portion.
>
> Fair. Attached is a new patch set which contains a few smaller commits
> that reorder things in xlog.c, and then the big commit that moves things
> to xlogrecovery.c.
>
> > If we're refactoring all of this, can we move the apply-one-record part into
> > its own function as well? Happy to do that as a followup or precursor patch
> > too. The per-record logic has grown complicated enough to make that quite
> > worthwhile imo - and imo most of the time one either is interested in the
> > per-record work, or in the rest of the StartupXLog/PerformWalRecovery logic.
>
> Added a commit to do that, as a follow-up. Yeah, I agree that makes sense.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: libpq compression

[vignesh C]

On Wed, Jul 14, 2021 at 6:31 PM Daniil Zakhlystov
 wrote:
>
> **sorry for the noise, but I need to re-send the message because one of the 
> recipients is blocked on the pgsql-hackers for some reason**
>
> Hi!
>
> Done, the patch should apply to the current master now.
>
> Actually, I have an almost-finished version of the patch with the previously 
> requested asymmetric compression negotiation. I plan to attach it soon.

Thanks for providing the patch quickly, I have changed the status to
"Need Review".

Regards,
Vignesh

Re: Implementing Incremental View Maintenance

[vignesh C]

On Mon, May 17, 2021 at 10:08 AM Yugo NAGATA  wrote:
>
> On Fri, 7 May 2021 14:14:16 +0900
> Yugo NAGATA  wrote:
>
> > On Mon, 26 Apr 2021 16:03:48 +0900
> > Yugo NAGATA  wrote:
> >
> > > On Mon, 26 Apr 2021 15:46:21 +0900
> > > Yugo NAGATA  wrote:
> > >
> > > > On Tue, 20 Apr 2021 09:51:34 +0900
> > > > Yugo NAGATA  wrote:
> > > >
> > > > > On Mon, 19 Apr 2021 17:40:31 -0400
> > > > > Tom Lane  wrote:
> > > > >
> > > > > > Andrew Dunstan  writes:
> > > > > > > This patch (v22c) just crashed for me with an assertion failure on
> > > > > > > Fedora 31. Here's the stack trace:
> > > > > >
> > > > > > > #2  0x0094a54a in ExceptionalCondition
> > > > > > > (conditionName=conditionName@entry=0xa91dae 
> > > > > > > "queryDesc->sourceText !=
> > > > > > > NULL", errorType=errorType@entry=0x99b468 "FailedAssertion",
> > > > > > > fileName=fileName@entry=0xa91468
> > > > > > > "/home/andrew/pgl/pg_head/src/backend/executor/execMain.c",
> > > > > > > lineNumber=lineNumber@entry=199) at
> > > > > > > /home/andrew/pgl/pg_head/src/backend/utils/error/assert.c:69
> > > > > >
> > > > > > That assert just got added a few days ago, so that's why the patch
> > > > > > seemed OK before.
> > > > >
> > > > > Thank you for letting me know. I'll fix it.
> > > >
> > > > Attached is the fixed patch.
> > > >
> > > > queryDesc->sourceText cannot be NULL after commit b2668d8,
> > > > so now we pass an empty string "" for refresh_matview_datafill() 
> > > > instead NULL
> > > > when maintaining views incrementally.
> > >
> > > I am sorry, I forgot to include a fix for 8aba9322511.
> > > Attached is the fixed version.
> >
> > Attached is the rebased patch (for 6b8d29419d).
>
> I attached a rebased patch.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Removing unneeded self joins

[vignesh C]

On Thu, May 27, 2021 at 12:21 PM Andrey V. Lepikhov
 wrote:
>
> On 5/8/21 2:00 AM, Hywel Carver wrote:
> > On Fri, May 7, 2021 at 8:23 AM Andrey Lepikhov
> > mailto:a.lepik...@postgrespro.ru>> wrote:
> > Here I didn't work on 'unnecessary IS NOT NULL filter'.
> >
> > I've tested the new patch, and it is giving the same improved behaviour
> > as the old patch.
> Thank you for this efforts!
>
> I cleaned the code of previous version, improved regression tests and
> rebased on current master.
>
> Also, I see that we could do additional optimizations for an
> EC-generated selfjoin clause (See equivclass.patch for necessary
> changes). Example:
> explain (costs off)
> select * from sj t1, sj t2 where t1.a = t1.b and t1.b = t2.b and t2.b =
> t2.a;
>   QUERY PLAN
> -
>   Seq Scan on sj t2
> Filter: ((a IS NOT NULL) AND (b = a) AND (a = b))
> (2 rows)
>
> But I'm not sure that this patch need to be a part of the self-join
> removal feature because of code complexity.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: postgres_fdw - make cached connection functions tests meaningful

[vignesh C]

On Mon, May 10, 2021 at 6:03 PM Bharath Rupireddy
 wrote:
>
> Hi,
>
> While working on [1], I got to know that there is a new GUC
> debug_invalidate_system_caches_always that has been introduced in v14.
> It can be used to switch off cache invalidation in
> CLOBBER_CACHE_ALWAYS builds which makes cache sensitive tests stable.
> Using this GUC, it is quite possible to make cached connection
> management function tests more meaningful by returning original
> values(true/false, all the output columns) instead of SELECT 1. Note
> that the commit f77717b29 stabilized the tests for those functions -
> postgres_fdw_disconnect, postgres_fdw_disconnect_all and
> postgres_fdw_get_connections by masking actual return value of the
> functions.
>
> Attaching a patch to use the new GUC to make the functions return actual 
> output.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Identify missing publications from publisher while create/alter subscription.

[vignesh C]

On Tue, Jul 6, 2021 at 8:09 PM vignesh C  wrote:
>
> On Wed, Jun 30, 2021 at 8:23 PM vignesh C  wrote:
> >
> > On Sun, Jun 6, 2021 at 11:55 AM vignesh C  wrote:
> > >
> > > On Fri, May 7, 2021 at 6:44 PM vignesh C  wrote:
> > > >
> > > > Thanks for the comments, the attached patch has the fix for the same.
> > >
> > > The patch was not applying on the head, attached patch which is rebased 
> > > on HEAD.
> >
> > The patch was not applying on the head, attached patch which is rebased on 
> > HEAD.
>
> The patch was not applying on the head, attached patch which is rebased on 
> HEAD.

The patch was not applying on the head, attached patch which is rebased on HEAD.

Regards,
Vignesh
From 09bd61d8ebcc4b5119048aeb7274bff2bdcae794 Mon Sep 17 00:00:00 2001
From: vignesh 
Date: Thu, 15 Jul 2021 17:39:52 +0530
Subject: [PATCH v11] Identify missing publications from publisher while
 create/alter subscription.

Creating/altering subscription is successful when we specify a publication which
does not exist in the publisher. This patch checks if the specified publications
are present in the publisher and throws an error if any of the publication is
missing in the publisher.
---
 doc/src/sgml/ref/alter_subscription.sgml  |  13 ++
 doc/src/sgml/ref/create_subscription.sgml |  18 +-
 src/backend/commands/subscriptioncmds.c   | 206 +++---
 src/bin/psql/tab-complete.c   |   7 +-
 src/test/subscription/t/007_ddl.pl|  68 ++-
 5 files changed, 280 insertions(+), 32 deletions(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index a6f994450d..e6b20542f9 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -166,6 +166,19 @@ ALTER SUBSCRIPTION name RENAME TO <
  
 

+
+   
+validate_publication (boolean)
+
+ 
+  When true, the command verifies if all the specified publications
+  that are being subscribed to are present in the publisher and throws
+  an error if any of the publication doesn't exist. The default is
+  false.
+ 
+
+   
+
   
 

diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 143390593d..194f0b977a 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -207,8 +207,9 @@ CREATE SUBSCRIPTION subscription_nameCREATE SUBSCRIPTION
   should connect to the publisher at all.  Setting this to
   false will change default values of
-  enabled, create_slot and
-  copy_data to false.
+  enabled, create_slot,
+  copy_data and
+  validate_publication to false.
  
 
  
@@ -276,6 +277,19 @@ CREATE SUBSCRIPTION subscription_name

+
+   
+validate_publication (boolean)
+
+ 
+  When true, the command verifies if all the specified publications
+  that are being subscribed to are present in the publisher and throws
+  an error if any of the publication doesn't exist. The default is
+  false.
+ 
+
+   
+
   
 

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 239d263f83..474fb92bbe 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -60,6 +60,7 @@
 #define SUBOPT_BINARY0x0080
 #define SUBOPT_STREAMING			0x0100
 #define SUBOPT_TWOPHASE_COMMIT		0x0200
+#define SUBOPT_VALIDATE_PUB			0x0400
 
 /* check if the 'val' has 'bits' set */
 #define IsSet(val, bits)  (((val) & (bits)) == (bits))
@@ -81,6 +82,7 @@ typedef struct SubOpts
 	bool		binary;
 	bool		streaming;
 	bool		twophase;
+	bool		validate_publication;
 } SubOpts;
 
 static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
@@ -128,6 +130,8 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 		opts->streaming = false;
 	if (IsSet(supported_opts, SUBOPT_TWOPHASE_COMMIT))
 		opts->twophase = false;
+	if (IsSet(supported_opts, SUBOPT_VALIDATE_PUB))
+		opts->validate_publication = false;
 
 	/* Parse options */
 	foreach(lc, stmt_options)
@@ -245,6 +249,15 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 			opts->specified_opts |= SUBOPT_TWOPHASE_COMMIT;
 			opts->twophase = defGetBoolean(defel);
 		}
+		else if (IsSet(supported_opts, SUBOPT_VALIDATE_PUB) &&
+ strcmp(defel->defname, "validate_publication") == 0)
+		{
+			if (IsSet(opts->specified_opts, SUBOPT_VALIDATE_PUB))
+errorConflictingDefElem(defel, pstate);
+
+			opts->specified_opts |= SUBOPT_VALIDATE_PUB;
+			opts->validate_publication = defGetBoolean

Re: A qsort template

[vignesh C]

On Sun, Jul 4, 2021 at 9:58 AM Thomas Munro  wrote:
>
> On Fri, Jul 2, 2021 at 2:32 PM John Naylor  
> wrote:
> > I suspect if we experiment on two extremes of type "heaviness" (accessing 
> > and comparing trivial or not), such as uint32 and tuplesort, we'll have a 
> > pretty good idea what the parameters should be, if anything different. I'll 
> > do some testing along those lines.
>
> Cool.
>
> Since you are experimenting with tuplesort and likely thinking similar
> thoughts, here's a patch I've been using to explore that area.  I've
> seen it get, for example, ~1.18x speedup for simple index builds in
> favourable winds (YMMV, early hacking results only).  Currently, it
> kicks in when the leading column is of type int4, int8, timestamp,
> timestamptz, date or text + friends (when abbreviatable, currently
> that means "C" and ICU collations only), while increasing the
> executable by only 8.5kB (Clang, amd64, -O2, no debug).
>
> These types are handled with just three specialisations.  Their custom
> "fast" comparators all boiled down to comparisons of datum bits,
> varying only in signedness and width, so I tried throwing them away
> and using 3 new common routines.  Then I extended
> tuplesort_sort_memtuples()'s pre-existing specialisation dispatch to
> recognise qualifying users of those and select 3 corresponding sort
> specialisations.
>
> It might turn out to be worth burning some more executable size on
> extra variants (for example, see XXX notes in the code comments for
> opportunities; one could also go nuts trying smaller things like
> special cases for not-null, nulls first, reverse sort, ... to kill all
> those branches), or not.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Rename of triggers for partitioned tables

[vignesh C]

On Mon, Jun 28, 2021 at 3:46 PM Arne Roland  wrote:
>
> Hi!
>
>
> From: Zhihong Yu 
> Sent: Saturday, June 26, 2021 20:32
> Subject: Re: Rename of triggers for partitioned tables
>
> > Hi, Arne:
> > It seems the patch no longer applies cleanly on master branch.
> > Do you mind updating the patch ?
> >
> > Thanks
>
> Thank you for having a look! Please let me know if the attached rebased patch 
> works for you.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Remove redundant initializations

[vignesh C]

On Mon, Jun 28, 2021 at 3:30 PM Peter Eisentraut
 wrote:
>
> There are certain parts of code that laboriously initialize every field
> of a struct to (some spelling of) zero, even though the whole struct was
> just zeroed (by makeNode() or memset()) a few lines earlier.  Besides
> being redundant, I find this hard to read in some situations because
> it's then very hard to tell what is different between different cases or
> branches.  The attached patch cleans up most of that.  I left alone
> instances where there are (nontrivial) comments attached to the
> initializations or where there appeared to be some value in maintaining
> symmetry.  But a lot of it was just plain useless code, some clearly
> copy-and-pasted repeatedly.
>
> Note
> 
> where we had a previous discussion about trimming down useless
> initializations to zero.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: logical decoding and replication of sequences

[vignesh C]

On Wed, Jun 23, 2021 at 7:55 PM Tomas Vondra
 wrote:
>
> On 6/23/21 4:14 PM, Tomas Vondra wrote:
> > A rebased patch, addressing a minor bitrot due to 4daa140a2f5.
> >
>
> Meh, forgot to attach the patch as usual, of course ...

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: Corrected documentation of data type for the logical replication message formats.

[vignesh C]

On Mon, Aug 2, 2021 at 9:10 PM Tom Lane  wrote:
>
> Peter Smith  writes:
> > I agree. The specified value looks better when it comes first, as you did 
> > it.
>
> Actually, it looks to me like we don't have to resolve the question of
> which should come first, because I don't see any cases where it's
> useful to have both.  I don't agree with appending "uint8" to those
> field descriptions, because it's adding no information, especially
> when the high bit couldn't be set anyway.
>
> At some point it might be useful to add UInt to the set of base
> data types, and then go through all the message types and decide
> which fields we think are unsigned.  But that is not this patch,
> and there would be questions about whether it constituted a protocol
> break.
>
> I noticed also that having to add "(Oid)" was sort of self-inflicted
> damage, because the field descriptions were using the very vague
> term "ID", when they could have said "OID" and been clear.  I left
> the "(Oid)" additions in place but also changed the text.
>
> Pushed with those changes.  I couldn't resist copy-editing the section
> intro, too.

Thanks for pushing the patch.

Regards,
Vignesh

Re: [PATCH]Comment improvement in publication.sql

[vignesh C]

On Mon, Aug 2, 2021 at 3:31 PM tanghy.f...@fujitsu.com
 wrote:
>
> Hi Hackers
>
> When review and test another patch at [1], I found some comments in existing 
> test code of " src/test/regress/sql/publication.sql " is a little bit 
> confused.
> Attached a patch to fix them, please take a check.
>
> Here is the detail:
>
> Existing code:
> CREATE TABLE testpub_tbl2 (id serial primary key, data text);
> -- fail - can't add to for all tables publication
> ALTER PUBLICATION testpub_foralltables ADD TABLE testpub_tbl2;
> -- fail - can't drop from all tables publication
> ALTER PUBLICATION testpub_foralltables DROP TABLE testpub_tbl2;
> -- fail - can't add to for all tables publication
> ALTER PUBLICATION testpub_foralltables SET TABLE pub_test.testpub_nopk;
>
> After patch:
> CREATE TABLE testpub_tbl2 (id serial primary key, data text);
> -- fail - tables can't be added to or dropped form FOR ALL TABLES publications
> ALTER PUBLICATION testpub_foralltables ADD TABLE testpub_tbl2;
> ALTER PUBLICATION testpub_foralltables DROP TABLE testpub_tbl2;
> ALTER PUBLICATION testpub_foralltables SET TABLE pub_test.testpub_nopk;
>
> You see the comment for SET TABLE is not appropriate.
> And above three operations(ADD DROP SET) output the same message as below:
> "DETAIL:  Tables cannot be added to or dropped from FOR ALL TABLES 
> publications."
>
> So maybe we can combine the existing three comments to one, thoughts?
>
> Besides, another comment in the same file is not clear enough to me:
> -- fail - already added
> CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1;
>
> Maybe it will be better if we use 'already exists'. Thoughts?
>
> [1] 
> https://www.postgresql.org/message-id/OS0PR01MB6113CC160D0F134448567FDDFBE99%40OS0PR01MB6113.jpnprd01.prod.outlook.com

Few minor suggestions:
1) Should we change below to "fail - tables can't be added, dropped or
set to "FOR ALL TABLES" publications"
--- fail - can't add to for all tables publication
+-- fail - tables can't be added to or dropped from FOR ALL TABLES publications

2) Should we change this to "--fail - already existing publication"
--- fail - already added
+-- fail - already exists

Regards,
Vignesh

Re: Failed transaction statistics to measure the logical replication progress

[vignesh C]

On Mon, Aug 2, 2021 at 1:13 PM Masahiko Sawada  wrote:
>
> On Mon, Aug 2, 2021 at 2:52 PM osumi.takami...@fujitsu.com
>  wrote:
> >
> > On Thursday, July 29, 2021 10:50 AM Masahiko Sawada  
> > wrote:
> > > On Thu, Jul 8, 2021 at 3:55 PM osumi.takami...@fujitsu.com
> > >  wrote:
> > > > When the current HEAD fails during logical decoding, the failure
> > > > increments txns count in pg_stat_replication_slots - [1] and adds
> > > > the transaction size to the sum of bytes in the same repeatedly on
> > > > the publisher, until the problem is solved.
> > > > One of the good examples is duplication error on the subscriber side
> > > > and this applies to both streaming and spill cases as well.
> > > >
> > > > This update prevents users from grasping the exact number and size
> > > > of successful and unsuccessful transactions. Accordingly, we need to
> > > > have new columns of failed transactions that will work to
> > > > differentiate both of them for all types, which means spill,
> > > > streaming and normal transactions. This will help users to measure
> > > > the exact status of logical replication.
> > >
> > > Could you please elaborate on use cases of the proposed statistics?
> > > For example, the current statistics on pg_replication_slots can be
> > > used for tuning logical_decoding_work_mem as well as inferring the
> > > total amount of bytes passed to the output plugin. How will the user use 
> > > those statistics?
> > >
> > > Also, if we want the stats of successful transactions why don't we
> > > show the stats of successful transactions in the view instead of ones
> > > of failed transactions?
> > It works to show the ratio of successful and unsuccessful transactions,
> > which should be helpful in terms of administrator perspective.
> > FYI, the POC patch added the columns where I prefixed 'failed' to those 
> > names.
> > But, substantially, it meant the ratio when user compared normal columns and
> > newly introduced columns by this POC in the pg_stat_replication_slots.
>
> What can the administrator use the ratio of successful and
> unsuccessful logical replication transactions for? For example, IIUC
> if a conflict happens on the subscriber as you mentioned, the
> successful transaction ratio calculated by those statistics is getting
> low, perhaps meaning the logical replication stopped. But it can be
> checked also by checking pg_stat_replication view or
> pg_replication_slots view (or error counts of
> pg_stat_subscription_errors view I’m proposing[1]). Do you have other
> use cases?

We could also include failed_data_size, this will help us to identify
the actual bandwidth consumed by the failed transaction. It will help
the DBA's to understand the network consumption in a better way.
Currently only total transaction and total data will be available but
when there is a failure, the failed transaction data will be sent
repeatedly, if the DBA does not solve the actual cause of failure,
there can be significant consumption of the network due to failure
transaction being sent repeatedly. DBA will not be able to understand
why there is so much network bandwidth consumption. If we give the
failed transaction information, the DBA might not get alarmed in this
case and understand that the network consumption is genuine. Also it
will help monitoring tools to project this value.

Regards,
Vignesh

Re: [HACKERS] logical decoding of two-phase transactions

[vignesh C]

On Tue, Aug 3, 2021 at 12:32 PM Amit Kapila  wrote:
>
> On Tue, Aug 3, 2021 at 6:17 AM Peter Smith  wrote:
> >
> > Please find attached the latest patch set v102*
> >
>
> I have made minor modifications in the comments and docs, please see
> attached. Can you please check whether the names of contributors in
> the commit message are correct or do we need to change it?

The patch applies neatly, the tests pass and documentation built with
the updates provided. I could not find any comments. The patch looks
good to me.

Regards,
Vignesh

Re: Skipping logical replication transactions on subscriber side

[vignesh C]

On Tue, Aug 3, 2021 at 12:20 PM Masahiko Sawada  wrote:
>
> On Mon, Aug 2, 2021 at 12:21 PM Amit Kapila  wrote:
> >
> > On Mon, Aug 2, 2021 at 7:45 AM Masahiko Sawada  
> > wrote:
> > >
> > > On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila  
> > > wrote:
> > > >
> > > > On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada 
> > > >  wrote:
> > > >
> > > > Setting up logical rep error context in a generic function looks a bit
> > > > odd to me. Do we really need to set up error context here? I
> > > > understand we can't do this in caller but anyway I think we are not
> > > > sending this to logical replication view as well, so not sure we need
> > > > to do it here.
> > >
> > > Yeah, I'm not convinced of this part yet. I wanted to show relid also
> > > in truncate cases but I came up with only this idea.
> > >
> > > If an error happens during truncating the table (in
> > > ExecuteTruncateGuts()), relid set by
> > > set_logicalrep_error_context_rel() is actually sent to the view. If we
> > > don’t have it, the view always shows relid as NULL in truncate cases.
> > > On the other hand, it doesn’t cover all cases. For example, it doesn’t
> > > cover an error that the target table doesn’t exist on the subscriber,
> > > which happens when opening the target table. Anyway, in most cases,
> > > even if relid is NULL, the error message in the view helps users to
> > > know which relation the error happened on. What do you think?
> > >
> >
> > Yeah, I also think at this stage error message is sufficient in such cases.
>
> I've attached new patches that incorporate all comments I got so far.
> Please review them.

I had a look at the first patch, couple of minor comments:
1) Should we include this in typedefs.lst
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+   LogicalRepMsgType command;  /* 0 if invalid */
+
+   /* Local relation information */
+   char   *nspname;

2)  We can keep the case statement in the same order as in the
LogicalRepMsgType enum, this will help in easily identifying if any
enum gets missed.
+   case LOGICAL_REP_MSG_RELATION:
+   return "RELATION";
+   case LOGICAL_REP_MSG_TYPE:
+   return "TYPE";
+   case LOGICAL_REP_MSG_ORIGIN:
+   return "ORIGIN";
+   case LOGICAL_REP_MSG_MESSAGE:
+   return "MESSAGE";
+   case LOGICAL_REP_MSG_STREAM_START:
+   return "STREAM START";

Regards,
Vignesh

Re: [PATCH]Comment improvement in publication.sql

[vignesh C]

On Tue, Aug 3, 2021 at 8:36 AM tanghy.f...@fujitsu.com
 wrote:
>
> On Monday, August 2, 2021 11:56 PM vignesh C  wrote:
> >
> > Few minor suggestions:
> > 1) Should we change below to "fail - tables can't be added, dropped or
> > set to "FOR ALL TABLES" publications"
> > --- fail - can't add to for all tables publication
> > +-- fail - tables can't be added to or dropped from FOR ALL TABLES 
> > publications
>
> Thanks for your kindly comments.
>
> I'm not sure should we ignore that 'drop' should be followed by 'from', but I
> think there's no misunderstandings. So, modified as you described.
>
> Besides, in other file comments (src/test/subscription/t/100_bugs.pl) I saw 
> FOR ALL TABLES without using quotes.
> So I don't think we need the quotes, too.
>
> > 2) Should we change this to "--fail - already existing publication"
> > --- fail - already added
> > +-- fail - already exists
>
> Modified.
>
> A modified patch is attached.

Thanks for the updated patch, the changes look good to me.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Aug 6, 2021 at 2:00 PM Masahiko Sawada  wrote:
>
> On Wed, Aug 4, 2021 at 12:08 AM vignesh C  wrote:
> >
> > On Tue, Aug 3, 2021 at 12:00 PM tanghy.f...@fujitsu.com
> >  wrote:
> > >
> > > On Monday, August 2, 2021 11:40 PM vignesh C wrote:
> > > >
> > > > Thanks for the comments, attached v17 patches has the fixes for the 
> > > > same.
> > >
> > > Thanks for your new patch.
> > >
> > > I saw the following warning when compiling. It seems we don't need this 
> > > variable any more.
> > >
> > > publicationcmds.c: In function ‘AlterPublicationSchemas’:
> > > publicationcmds.c:592:15: warning: unused variable ‘oldlc’ 
> > > [-Wunused-variable]
> > >ListCell   *oldlc;
> > >^
> >
> > Thanks for reporting this, this is fixed in the v18 patch attached.
>
> I've also started reviewing this patch. I've not looked at the patch
> yet but here are initial comments/questions based on using this
> feature:
>
> pg_publication catalog still has puballtables column but it's still
> necessary? IIUC since pubtype = 'a' means publishing all tables in the
> database puballtables seems no longer necessary.

I will remove puballtables  in my next version of the patch.

> ---
> Suppose that a parent table and its child table are defined in
> different schemas, there is a publication for the schema where only
> the parent table is defined, and the subscriber subscribes to the
> publication, should changes for its child table be replicated to the
> subscriber?

I felt that in this case only the table data that is present in the
publish schema should be sent to the subscriber. Since the child table
schema is not part of the publication, I felt this child table data
should not be replicated. Thoughts?
I have kept the above same behavior in the case of publication created
using PUBLISH_VIA_PARTITION_ROOT option i.e the child table data will
not be sent.  But now I'm feeling we should send the child table data
since it is being sent through the parent table which is part of the
publication. Also this way users can use this option if the user has
the table having partitions designed across the schemas. Thoughts?

> In FOR TABLE cases, i.g., where the subscriber subscribes to the
> publication that is only for the parent table, changes for its child
> table are replicated to the subscriber.
>
> As far as I tested v18 patch, changes for the child table are not
> replicated in FOR SCHEMA cases. Here is the test script:
>
> On publisher and subscriber:
> create schema p_schema;
> create schema c_schema;
> create table p_schema.p (a int) partition by list (a);
> create table c_schema.c partition of p_schema.p for values in (1);
>
> On publisher:
> create publication pub_p_schema for schema p_schema;
>
> On subscriber:
> create subscription pub connection 'dbname=postgres' publication pub_p_schema;
>
> On publisher:
> insert into p_schema.p values (1);
> select * from p_schema.p;
>  a
> ---
>  1
> (1 row)
>
> On subscriber:
> select * from p_schema.p;
>  a
> ---
>
> (0 rows)

I have kept this behavior intentionally, details explained above. Thoughts?

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Aug 4, 2021 at 8:08 PM tanghy.f...@fujitsu.com <
tanghy.f...@fujitsu.com> wrote:
>
> On Tuesday, August 3, 2021 11:08 PM vignesh C  wrote:
> >
> > Thanks for reporting this, this is fixed in the v18 patch attached.
>
> Thanks for fixing it.
>
> Few suggestions for V18:
>
> 1.
> +# Clean up the tables on both publisher and subscriber as we don't need
them
> +$node_publisher->safe_psql('postgres', "DROP SCHEMA sch1 cascade");
> +$node_publisher->safe_psql('postgres', "DROP SCHEMA sch2 cascade");
> +$node_publisher->safe_psql('postgres', "DROP SCHEMA sch3 cascade");
> +$node_subscriber->safe_psql('postgres', "DROP SCHEMA sch1 cascade");
> +$node_subscriber->safe_psql('postgres', "DROP SCHEMA sch2 cascade");
> +$node_subscriber->safe_psql('postgres', "DROP SCHEMA sch3 cascade");
>
> Should we change the comment to "Clean up the schemas ... ", instead of
'tables'?

Modified.

> 2.
> +$result = $node_subscriber->safe_psql('postgres',
> +"SELECT count(*) FROM sch1.tab3");
>
> Spaces are used here(and some other places), but in most places we use a
TAB, so
> I think it's better to change it to a TAB.

Modified.

> 3.
> List   *tables; /* List of tables to
add/drop */
> boolfor_all_tables; /* Special publication for all
tables in db */
> DefElemAction tableAction;  /* What action to perform with
the tables */
> +   List   *schemas;/* Optional list of schemas */
>  } AlterPublicationStmt;
>
> Should we change the comment here to 'List of schemas to add/drop', then
it can
> be consistent with the comment for 'tables'.

Modified.

> I also noticed that 'tableAction' variable is used when we add/drop/set
schemas,
> so maybe the variable name is not suitable any more.

Changed the variable name.

> Besides, the following comment is above these codes. Should we add some
comments
> for schema?
>
> /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */

Modified

> And it says 'add/drop', do we need to add 'set'? (it's not related to this
> patch, so I think I can add it in another thread[1] if needed, which is
related
> to comment improvement)

You can include the change in the patch posted.

> 4.
> I saw the existing tests about permissions in publication.sql, should we
add
> tests for schema publication? Like this:
>
> diff --git a/src/test/regress/sql/publication.sql
b/src/test/regress/sql/publication.sql
> index 33dbdf7bed..c19337631e 100644
> --- a/src/test/regress/sql/publication.sql
> +++ b/src/test/regress/sql/publication.sql
> @@ -160,16 +160,19 @@ GRANT CREATE ON DATABASE regression TO
regress_publication_user2;
>  SET ROLE regress_publication_user2;
>  SET client_min_messages = 'ERROR';
>  CREATE PUBLICATION testpub2;  -- ok
> +CREATE PUBLICATION testpub3;  -- ok
>  RESET client_min_messages;
>
>  ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1;  -- fail
> +ALTER PUBLICATION testpub3 ADD SCHEMA pub_test;  -- fail
>
>  SET ROLE regress_publication_user;
>  GRANT regress_publication_user TO regress_publication_user2;
>  SET ROLE regress_publication_user2;
>  ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1;  -- ok
> +ALTER PUBLICATION testpub3 ADD SCHEMA pub_test;  -- ok
>
> -DROP PUBLICATION testpub2;
> +DROP PUBLICATION testpub2, testpub3;
>
>  SET ROLE regress_publication_user;
>  REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;

Added.

The changes for the above are available in the v19 patch posted at [1].

[1] -
https://www.postgresql.org/message-id/CALDaNm3BMLBpWOSdS3Q2vwpsM%3D0yovpJm8dKHRqNyFpANbrhpw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Thu, Aug 5, 2021 at 3:54 PM Amit Kapila  wrote:
>
> On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila 
wrote:
> >
> > On Tue, Aug 3, 2021 at 8:38 PM vignesh C  wrote:
> > >
> > > Thanks for reporting this, this is fixed in the v18 patch attached.
> > >
> >
> > I have started looking into this patch and below are some initial
comments.
> >
>
> Few more comments:
> ===
> 1. Do we need the previous column 'puballtables' after adding pubtype
> or pubkind in pg_publication?

I felt this should be retained as old client will still be using
puballtables, like in case of old client executing \dRp+  commands.

> 2.
> @@ -224,6 +279,20 @@ CreatePublication(ParseState *pstate,
> CreatePublicationStmt *stmt)
> ..
> + nspcrel = table_open(NamespaceRelationId, ShareUpdateExclusiveLock);
> + PublicationAddSchemas(puboid, schemaoidlist, true, NULL);
> + table_close(nspcrel, ShareUpdateExclusiveLock);
>
> What is the reason for opening and taking a lock on
> NamespaceRelationId? Do you want to avoid dropping the corresponding
> schema during this duration? If so, that is not sufficient because
> what if somebody drops it just before you took lock on
> NamespaceRelationId. I think you need to use LockDatabaseObject to
> avoid dropping the schema and note that it should be unlocked only at
> the end of the transaction similar to what we do for tables. I guess
> you need to add this locking inside the function
> PublicationAddSchemas. Also, it would be good if you can add few
> comments in this part of the code to explain the reason for locking.

Modified.

> 3. The function PublicationAddSchemas is called from multiple places
> in the patch but the locking protection is not there at all places. I
> think if you add locking as suggested in the previous point then it
> should be okay. I think you need similar locking for
> PublicationDropSchemas.

Modified.

> 4.
> @@ -421,16 +537,84 @@ AlterPublicationTables(AlterPublicationStmt
> *stmt, Relation rel,
>   PublicationAddTables(pubid, rels, true, stmt);
>
>   CloseTableList(delrels);
> + if (pubform->pubtype == PUBTYPE_EMPTY)
> + UpdatePublicationTypeTupleValue(rel, tup,
> + Anum_pg_publication_pubtype,
> + PUBTYPE_TABLE);
>   }
>
> At the above and all other similar places, the patch first updates the
> pg_publication after performing the rel/schema operation. Isn't it
> better to first update pg_publication to remain in sync with how
> CreatePublication works? I am not able to see any issue with the way
> you have it in the patch but it is better to keep the code consistent
> across various similar functions to avoid confusion in the future.

Modified.

Thanks for the comments, the changes for the above are available in the v19
patch posted at [1].

[1] -
https://www.postgresql.org/message-id/CALDaNm3BMLBpWOSdS3Q2vwpsM%3D0yovpJm8dKHRqNyFpANbrhpw%40mail.gmail.com

Regards,
Vignesh

Re: [PATCH]Comment improvement in publication.sql

[vignesh C]

On Fri, Aug 6, 2021 at 3:33 PM tanghy.f...@fujitsu.com
 wrote:
>
> Hi
>
> I saw some inaccurate comments for AlterPublicationStmt structure when
> reviewing patches related to publication[1].
>
> The variable tables are used for 'ALTER PUBLICATION ... ADD/DROP/SET TABLE',
> but the comments only say 'ADD/DROP'. How about add 'SET' to the comments?
>
> typedef struct AlterPublicationStmt
> {
> NodeTag type;
> char   *pubname;/* Name of the publication */
>
> /* parameters used for ALTER PUBLICATION ... WITH */
> List   *options;/* List of DefElem nodes */
>
> /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */
> List   *tables; /* List of tables to add/drop 
> */
> boolfor_all_tables; /* Special publication for all tables 
> in db */
> DefElemAction tableAction;  /* What action to perform with the 
> tables */
> } AlterPublicationStmt;
>
> It's also a comment improvement, so I add this change to this patch.

Thanks for the updated patch, your changes look good to me. You might
want to include the commit message in the patch, that will be useful.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Aug 6, 2021 at 4:39 PM Amit Kapila  wrote:
>
> On Fri, Aug 6, 2021 at 2:02 PM vignesh C  wrote:
> >
> > On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila  wrote:
> > >
> > > On Tue, Aug 3, 2021 at 8:38 PM vignesh C  wrote:
> >
> > > 6.
> > > + {PublicationSchemaRelationId, /* PUBLICATIONSCHEMAMAP */
> > > + PublicationSchemaPsnspcidPspubidIndexId,
> > > + 2,
> > > + {
> > > + Anum_pg_publication_schema_psnspcid,
> > > + Anum_pg_publication_schema_pspubid,
> > > + 0,
> > > + 0
> > > + },
> > >
> > > Why don't we keep pubid as the first column in this index?
> >
> > I wanted to keep it similar to PUBLICATIONRELMAP, should we keep it as
> > it is, thoughts?
> >
>
> Okay, I see your point. I think for PUBLICATIONRELMAP, we need it
> because it is searched using the only relid in
> GetRelationPublications, so, similarly, in the patch, you are using
> schema_oid in GetSchemaPublications, so probably that will help. I was
> wondering why you haven't directly used the cache in
> GetSchemaPublications similar to GetRelationPublications?

Both of the approaches work, I was not sure which one is better, If
the approach in GetRelationPublications is better, I will change it to
something similar to GetRelationPublications. Thoughts?

It seems
> there is a danger for concurrent object drop. Can you please check how
> the safety is ensured say when either one wants to drop the
> corresponding relation/schema or publication?

If a table is dropped concurrently from another session during logical
replication of some operation in that table, while we get
get_rel_sync_entry the cache invalidations(rel_sync_cache_relation_cb)
happen.  The cache entry will be marked as false, also the schema_sent
will be marked as false. It will resend the relation using the
relation that was prepared while processing this transaction from
ReorderBufferProcessTXN. I felt this is ok since the relation is
dropped after the operation on the table. Similarly if the publication
is dropped concurrently from another session during logical
replication of some operation in that table, while we get
get_rel_sync_entry the cache
invalidations(publication_invalidation_cb) happen. The publications
will be reloaded and validated again, the data will be replicated to
the server. I felt this behavior is fine since the publication is
dropped after the operation on the table.

 Another point is why
> can't we use the other index (where the index is on relid or
> schema_oid (PublicationSchemaObjectIndexId))?

I felt this cannot be used because In this case the index is in the
oid column of pg_publication_schema and not on psnspcid column.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Aug 9, 2021 at 9:50 PM Mark Dilger  wrote:
>
>
>
> > On Aug 6, 2021, at 1:32 AM, vignesh C  wrote:
> >
> > the attached v19 patch
>
> With v19 applied, a schema owner can publish the contents of a table 
> regardless of ownership or permissions on that table:
>
> +CREATE ROLE user1;
> +GRANT CREATE ON DATABASE regression TO user1;
> +CREATE ROLE user2;
> +GRANT CREATE ON DATABASE regression TO user2;
> +SET SESSION AUTHORIZATION user1;
> +CREATE SCHEMA user1schema;
> +GRANT CREATE, USAGE ON SCHEMA user1schema TO user2;
> +RESET SESSION AUTHORIZATION;
> +SET SESSION AUTHORIZATION user2;
> +CREATE TABLE user1schema.user2private (junk text);
> +REVOKE ALL PRIVILEGES ON user1schema.user2private FROM PUBLIC;
> +REVOKE ALL PRIVILEGES ON user1schema.user2private FROM user1;
> +CREATE TABLE user1schema.user2public (junk text);
> +GRANT SELECT ON user1schema.user2public TO PUBLIC;
> +RESET SESSION AUTHORIZATION;
> +SET SESSION AUTHORIZATION user1;
> +SELECT * FROM user1schema.user2private;
> +ERROR:  permission denied for table user2private
> +SELECT * FROM user1schema.user2public;
> + junk
> +--
> +(0 rows)
> +
> +CREATE PUBLICATION user1pub;
> +WARNING:  wal_level is insufficient to publish logical changes
> +HINT:  Set wal_level to logical before creating subscriptions.
> +ALTER PUBLICATION user1pub
> +   ADD TABLE user1schema.user2public;
> +ERROR:  must be owner of table user2public
> +ALTER PUBLICATION user1pub
> +   ADD TABLE user1schema.user2private, user1schema.user2public;
> +ERROR:  must be owner of table user2private
> +SELECT * FROM pg_catalog.pg_publication_tables
> +   WHERE pubname = 'user1pub';
> + pubname | schemaname | tablename
> +-++---
> +(0 rows)
> +
> +ALTER PUBLICATION user1pub ADD SCHEMA user1schema;
> +SELECT * FROM pg_catalog.pg_publication_tables
> +   WHERE pubname = 'user1pub';
> + pubname  | schemaname  |  tablename
> +--+-+--
> + user1pub | user1schema | user2private
> + user1pub | user1schema | user2public
> +(2 rows)
>
> It is a bit counterintuitive that schema owners do not have administrative 
> privileges over tables within their schemas, but that's how it is.  The 
> design of this patch seems to assume otherwise.  Perhaps ALTER PUBLICATION 
> ... ADD SCHEMA should be restricted to superusers, just as FOR ALL TABLES?

I will handle this in the next version of the patch.
Additionally I will add this check for "Alter publication add schema"
and "Alter publication set schema". I'm not planning to add this check
for "Alter publication drop schema" to keep the behavior similar to
"Alter publication drop table".
Also, the behavior of "Alter publication drop table" for which the
user is not the owner is successful, Is this behavior correct?
create table tbl1(c1 int);
create table tbl2(c1 int);
create publication mypub1 for table tbl1,tbl2;
SET SESSION AUTHORIZATION user2;
alter table tbl2 owner to user2;
RESET SESSION AUTHORIZATION;
postgres=> alter publication mypub1  drop table tbl2;
ALTER PUBLICATION
postgres=> alter publication mypub1 add table tbl2;
ERROR:  must be owner of table tbl2

Thoughts?

Regards,
Vignesh

Re: [HACKERS] logical decoding of two-phase transactions

[vignesh C]

On Fri, Jul 30, 2021 at 9:32 AM Peter Smith  wrote:
>
> Please find attached the latest patch set v100*
>
> v99-0002 --> v100-0001
>
> Differences:
>
> * Rebased to HEAD @ today (needed because some recent commits [1][2] broke 
> v99)
>

The patch applies neatly, tests passes and documentation looks good.
A Few minor comments.
1) This blank line is not required:
+-- two_phase and streaming are compatible.
+CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, streaming = true, two_phase = true);
+

2) Few points have punctuation mark and few don't have, we can make it
consistent:
+###
+# Test 2PC PREPARE / ROLLBACK PREPARED.
+# 1. Table is deleted back to 2 rows which are replicated on subscriber.
+# 2. Data is streamed using 2PC
+# 3. Do rollback prepared.
+#
+# Expect data rolls back leaving only the original 2 rows.
+###

3) similarly here too:
+###
+# Do INSERT after the PREPARE but before ROLLBACK PREPARED.
+# 1. Table is deleted back to 2 rows which are replicated on subscriber.
+# 2. Data is streamed using 2PC.
+# 3. A single row INSERT is done which is after the PREPARE
+# 4. Then do a ROLLBACK PREPARED.
+#
+# Expect the 2PC data rolls back leaving only 3 rows on the subscriber.
+# (the original 2 + inserted 1)
+###

Regards,
Vignesh

Re: [HACKERS] logical decoding of two-phase transactions

[vignesh C]

On Sat, Jul 31, 2021 at 11:12 AM Ajin Cherian  wrote:
>
> On Sat, Jul 31, 2021 at 2:39 PM Amit Kapila  wrote:
>
> > Here, the test is expecting 2 prepared transactions corresponding to
> > two subscriptions but it waits for just one subscription via
> > appname_copy. It should wait for the second subscription using
> > $appname as well.
> >
> > What do you think?
>
> I agree with this analysis. The test needs to wait for both
> subscriptions to catch up.
> Attached is a patch that addresses this issue.

The changes look good to me.

Regards,
Vignesh

Re: Corrected documentation of data type for the logical replication message formats.

[vignesh C]

On Sat, Jul 31, 2021 at 2:30 AM Tom Lane  wrote:
>
> vignesh C  writes:
> [ v6-0001-Included-the-actual-datatype-used-in-logical-repl.patch ]
>
> I see what you want to do here, but the way you did it seems quite
> detrimental to the readability of the field descriptions.
> Parenthesized interjections should be used sparingly.
>
> I'm inclined to think that the equivalent data type is part of the
> field data type specification, and thus that we ought to put it in
> the data type part of each entry.  So we'd have something like
>
> 
> 
> Int64 (XLogRecPtr)
> 
> 
> 
> The final LSN of the transaction.
> 
> 
> 
>

I made changes based on the feedback, since Peter also was in favour
of using this approach, I modified based on the first approach.
Attached v7 patch has the changes for the same.

Regards,
Vignesh
From 285f4fe337222690f27f164d7210afbda84c37d0 Mon Sep 17 00:00:00 2001
From: Vigneshwaran c 
Date: Sun, 1 Aug 2021 20:21:31 +0530
Subject: [PATCH v7] Included the actual datatype used in logical replication
 message format documentation.

Included the actual datatype used in logical replication message format
documentation.
---
 doc/src/sgml/protocol.sgml | 159 +++--
 1 file changed, 100 insertions(+), 59 deletions(-)

diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 9843953b05..ac53c82d19 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -6411,7 +6411,8 @@ This section describes the detailed format of each logical replication message.
 These messages are returned either by the replication slot SQL interface or are
 sent by a walsender. In case of a walsender they are encapsulated inside the replication
 protocol WAL messages as described in 
-and generally obey same message flow as physical replication.
+and generally obey same message flow as physical replication. The base data types
+used are described in .
 
 
 
@@ -6436,7 +6437,7 @@ Begin
 
 
 
-Int64
+Int64 (XLogRecPtr)
 
 
 
@@ -6446,7 +6447,7 @@ Begin
 
 
 
-Int64
+Int64 (TimestampTz)
 
 
 
@@ -6457,7 +6458,7 @@ Begin
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -6491,7 +6492,7 @@ Message
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -6502,7 +6503,7 @@ Message
 
 
 
-Int8
+Int8 (uint8)
 
 
 
@@ -6513,7 +6514,7 @@ Message
 
 
 
-Int64
+Int64 (XLogRecPtr)
 
 
 
@@ -6579,17 +6580,17 @@ Commit
 
 
 
-Int8
+Int8(0) (uint8)
 
 
 
-Flags; currently unused (must be 0).
+Flags; currently unused.
 
 
 
 
 
-Int64
+Int64 (XLogRecPtr)
 
 
 
@@ -6599,7 +6600,7 @@ Commit
 
 
 
-Int64
+Int64 (XLogRecPtr)
 
 
 
@@ -6609,7 +6610,7 @@ Commit
 
 
 
-Int64
+Int64 (TimestampTz)
 
 
 
@@ -6644,7 +6645,7 @@ Origin
 
 
 
-Int64
+Int64 (XLogRecPtr)
 
 
 
@@ -6693,7 +6694,7 @@ Relation
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -6704,7 +6705,7 @@ Relation
 
 
 
-Int32
+Int32 (Oid)
 
 
 
@@ -6760,7 +6761,7 @@ Relation
 
 
 
-Int8
+Int8 (uint8)
 
 
 
@@ -6781,7 +6782,7 @@ Relation
 
 
 
-Int32
+Int32 (Oid)
 
 
 
@@ -6825,7 +6826,7 @@ Type
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -6836,7 +6837,7 @@ Type
 
 
 
-Int32
+Int32 (Oid)
 
 
 
@@ -6890,7 +6891,7 @@ Insert
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -6901,7 +6902,7 @@ Insert
 
 
 
-Int32
+Int32 (Oid)
 
 
 
@@ -6957,7 +6958,7 @@ Update
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -6968,7 +6969,7 @@ Update
 
 
 
-Int32
+Int32 (Oid)
 
 
 
@@ -7071,7 +7072,7 @@ Delete
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -7082,7 +7083,7 @@ Delete
 
 
 
-Int32
+Int32 (Oid)
 
 
 
@@ -7160,7 +7161,7 @@ Truncate
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -7192,7 +7193,7 @@ Truncate
 
 
 
-Int32
+Int32 (Oid)
 
 
 
@@ -7238,7 +7239,7 @@ Stream Start
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -7307,7 +7308,7 @@ Stream Commit
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -7317,17 +7318,17 @@ Stream Commit
 
 
 
-Int8
+Int8(0) (uint8)
 
 
 
-Flags; currently unused (must be 0).
+Flags; currently unused.
 
 
 
 
 
-Int64
+Int64 (XLogRecPtr)
 
 
 
@@ -7337,7 +7338,7 @@ Stream Commit
 
 
 
-Int64
+Int64 (XLogRecPtr)
 
 
 
@@ -7347,7 +7348,7 @@ Stream Commit
 
 
 
-Int64
+Int64 (TimestampTz)
 
 
 
@@ -7382,7 +7383,7 @@ Stream Abort
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -7392,7 +7393,7 @@ Stream Abort
 
 
 
-Int32
+Int32 (TransactionId)
 
 
 
@@ -7432,21 +7433,27 @@ 

Re: Corrected documentation of data type for the logical replication message formats.

[vignesh C]

On Sun, Aug 1, 2021 at 4:11 PM Peter Smith  wrote:
>
> On Sat, Jul 31, 2021 at 7:00 AM Tom Lane  wrote:
> >
> > vignesh C  writes:
> > [ v6-0001-Included-the-actual-datatype-used-in-logical-repl.patch ]
> >
> > I see what you want to do here, but the way you did it seems quite
> > detrimental to the readability of the field descriptions.
> > Parenthesized interjections should be used sparingly.
> >
> > I'm inclined to think that the equivalent data type is part of the
> > field data type specification, and thus that we ought to put it in
> > the data type part of each entry.  So we'd have something like
> >
> > 
> > 
> > Int64 (XLogRecPtr)
> > 
> > 
> > 
> > The final LSN of the transaction.
> > 
> > 
> > 
> >
> > instead of what you did here.  Parentheses might not be the best
> > punctuation to use, given the existing convention about parenthesized
> > specific values, but we could probably settle on some other markup.
> > Or just ignore the ambiguity.
>
> +1 to change it like suggested above.
>
> The specific value for the flags might then look like below, but that
> does not look too bad to me.
>
> 
> Int8 (uint8) (0)
> 

I felt we can change it like:

Int8(0) (uint8)


I felt the flag value can be kept first followed by the data type since it
is used similarly for the other message types like below:

Byte1('C')


I have made changes in similar lines and posted the patch at [1].
Thoughts?

[1] -
https://www.postgresql.org/message-id/CALDaNm3sK75Mo%2BVzLmNGe29gYtJoeKHshAK0GDiAzfAj6LQPdw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Aug 6, 2021 at 4:02 PM Amit Kapila  wrote:
>
> On Fri, Aug 6, 2021 at 2:16 PM vignesh C  wrote:
> >
> > On Thu, Aug 5, 2021 at 3:54 PM Amit Kapila  wrote:
> > >
> > >
> > > Few more comments:
> > > ===
> > > 1. Do we need the previous column 'puballtables' after adding pubtype
> > > or pubkind in pg_publication?
> >
> > I felt this should be retained as old client will still be using 
> > puballtables, like in case of old client executing \dRp+  commands.
> >
>
> But do we guarantee that old clients work with newer server versions?
> For example, psql docs say: "psql works best with servers of the same
> or an older major version. Backslash commands are particularly likely
> to fail if the server is of a newer version than psql itself." [1]
> (See Notes Section). Similarly, pg_dump docs say: "However, pg_dump
> cannot dump from PostgreSQL servers newer than its own major version;
> it will refuse to even try, rather than risk making an invalid dump."
> [2] (See Notes Section).
>
> It seems Sawada-San has the same question and IIUC docs suggest we
> don't need such compatibility, so what makes you think we need it?

Ok, I was not sure if we can remove any system table columns, hence
had retained it. It seems that my understanding was wrong. I will
remove this column in the next version of the patch.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Sep 20, 2021 at 4:20 PM vignesh C  wrote:
>
> On Mon, Sep 20, 2021 at 3:57 PM Amit Kapila  wrote:
> >
> > On Fri, Sep 17, 2021 at 5:40 PM vignesh C  wrote:
> > >
> > > On Thu, Sep 16, 2021 at 9:54 AM Amit Kapila  
> > > wrote:
> > > >
> > > > I think there is one more similar locking problem.
> > > > AlterPublicationSchemas()
> > > > {
> > > > ..
> > > > + if (stmt->action == DEFELEM_ADD)
> > > > + {
> > > > + List *rels;
> > > > +
> > > > + rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
> > > > + RelSchemaIsMemberOfSchemaList(rels, schemaidlist, true);
> > > > ...
> > > > ...
> > > > }
> > > >
> > > > Here, we don't have a lock on the relation. So, what if the relation
> > > > is concurrently dropped after you get the rel list by
> > > > GetPublicationRelations?
> > >
> > > This works fine without locking even after concurrent drop, I felt
> > > this works because of MVCC.
> > >
> >
> > Can you share the exact scenario you have tested? I think here it can
> > give a wrong error because it might access invalid cache entry, so I
> > think a lock is required here. Also, as said before, this might help
> > to make the rel list consistent in function
> > CheckObjSchemaNotAlreadyInPublication().
>
> This is the scenario I tried:
> create schema sch1;
> create table sch1.t1 (c1 int);
> create publication pub1 for table sch1.t1;
> alter publication pub1 add all tables in schema sch1;  -- concurrently
> drop table sch1.t1 from another session.
>
> I will add the locking and changing of
> CheckObjSchemaNotAlreadyInPublication in the next version.

I have made the changes for the above at the v30 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm3aFtXpkD4m28-ENG9F4faBEVdGNUrEhgKV0pHr2S_C2g%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Tue, Sep 21, 2021 at 9:03 AM Greg Nancarrow  wrote:
>
> On Fri, Sep 17, 2021 at 10:09 PM vignesh C  wrote:
> >
> > Attached v29 patch has the fixes for the same.
> >
>
> Some minor comments on the v29-0002 patch:
>
> (1)
> In get_object_address_publication_schema(), the error message:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\"
> does not exist",
>
> isn't grammatically correct. It should probably be:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\" do
> not exist",

Modified

> (2)
> getPublicationSchemaInfo() function header.
> "string" should be "strings"
>
> BEFORE:
> + * nspname. Both pubname and nspname are palloc'd string which will be freed 
> by
> AFTER:
> + * nspname. Both pubname and nspname are palloc'd strings which will
> be freed by

Modified

> (3)
> getPublicationSchemaInfo()
>
> In the case of "if (!(*nspname))", the following line should probably
> be added before returning false:
>
>*pubname = NULL;

I left it as it is, as we don't access it in case of return false.

> (4)
> GetAllSchemasPublicationRelations() function header
>
> Shouldn't:
>
> + * Gets the list of all relations published by FOR ALL TABLES IN SCHEMA
> + * publication(s).
>
> actually say:
>
> + * Gets the list of all relations published by a FOR ALL TABLES IN SCHEMA
> + * publication.
>
> since it is for a specified publication?

Modified

> (5)
> I'm wondering, in CheckObjSchemaNotAlreadyInPublication(), instead of
> checking "checkobjtype" each loop iteration, wouldn't it be better to
> just use the same for-loop in each IF block?

Modified

I have made the changes for the above at the v30 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm3aFtXpkD4m28-ENG9F4faBEVdGNUrEhgKV0pHr2S_C2g%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Sep 20, 2021 at 3:57 PM Amit Kapila  wrote:
>
> On Fri, Sep 17, 2021 at 5:40 PM vignesh C  wrote:
> >
> > On Thu, Sep 16, 2021 at 9:54 AM Amit Kapila  wrote:
> > >
> > > I think there is one more similar locking problem.
> > > AlterPublicationSchemas()
> > > {
> > > ..
> > > + if (stmt->action == DEFELEM_ADD)
> > > + {
> > > + List *rels;
> > > +
> > > + rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
> > > + RelSchemaIsMemberOfSchemaList(rels, schemaidlist, true);
> > > ...
> > > ...
> > > }
> > >
> > > Here, we don't have a lock on the relation. So, what if the relation
> > > is concurrently dropped after you get the rel list by
> > > GetPublicationRelations?
> >
> > This works fine without locking even after concurrent drop, I felt
> > this works because of MVCC.
> >
>
> Can you share the exact scenario you have tested? I think here it can
> give a wrong error because it might access invalid cache entry, so I
> think a lock is required here. Also, as said before, this might help
> to make the rel list consistent in function
> CheckObjSchemaNotAlreadyInPublication().

This is the scenario I tried:
create schema sch1;
create table sch1.t1 (c1 int);
create publication pub1 for table sch1.t1;
alter publication pub1 add all tables in schema sch1;  -- concurrently
drop table sch1.t1 from another session.

I will add the locking and changing of
CheckObjSchemaNotAlreadyInPublication in the next version.

Regards,
Vignesh

Re: Column Filtering in Logical Replication

[vignesh C]

On Fri, Sep 24, 2021 at 6:55 PM Alvaro Herrera  wrote:
>
> On 2021-Sep-23, Amit Kapila wrote:
>
> > Alvaro, do you have any thoughts on these proposed grammar changes?
>
> Yeah, I think pubobj_name remains a problem in that you don't know its
> return type -- could be a String or a RangeVar, and the user of that
> production can't distinguish.  So you're still (unnecessarily, IMV)
> stashing an object of undetermined type into ->object.
>
> I think you should get rid of both pubobj_name and pubobj_expr and do
> somethine like this:
>
> /* FOR TABLE and FOR ALL TABLES IN SCHEMA specifications */
> PublicationObjSpec: TABLE ColId
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_TABLE;
> $$->rangevar = 
> makeRangeVarFromQualifiedName($1, NULL, @1, yyscanner);
> }
> | TABLE ColId indirection
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_TABLE;
> $$->rangevar = 
> makeRangeVarFromQualifiedName($1, $2, @1, yyscanner);
> }
> | ALL TABLES IN_P SCHEMA ColId
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_REL_IN_SCHEMA;
> $$->name = $4;
> }
> | ALL TABLES IN_P SCHEMA CURRENT_SCHEMA /* XXX should 
> this be "IN_P CURRENT_SCHEMA"? */
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_CURRSCHEMA;
> $$->name = $4;
> }
> | ColId
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->name = $1;
> $$->pubobjtype = 
> PUBLICATIONOBJ_CONTINUATION;
> }
> | ColId indirection
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->rangevar = 
> makeRangeVarFromQualifiedName($1, $2, @1, yyscanner);
> $$->pubobjtype = 
> PUBLICATIONOBJ_CONTINUATION;
> }
> | CURRENT_SCHEMA
> {
> $$ = 
> makeNode(PublicationObjSpec);
> $$->pubobjtype = 
> PUBLICATIONOBJ_CURRSCHEMA;
> }
> ;

Apart from the issue that Hou San pointed, I found one issue with
introduction of PUBLICATIONOBJ_CURRSCHEMA, I was not able to
differentiate if it is table or schema in the following cases:
CREATE PUBLICATION pub1 FOR ALL TABLES IN SCHEMA CURRENT_SCHEMA;
CREATE PUBLICATION pub1 FOR ALL TABLES IN SCHEMA sch1, CURRENT_SCHEMA;
CREATE PUBLICATION pub1 FOR table t1, CURRENT_SCHEMA;
The differentiation is required to differentiate and add a schema or a table.

I felt it was better to use PUBLICATIONOBJ_CONTINUATION in case of
CURRENT_SCHEMA in multiple object cases like:
PublicationObjSpec: TABLE relation_expr
{
$$ =
makeNode(PublicationObjSpec);
$$->pubobjtype =
PUBLICATIONOBJ_TABLE;
$$->rangevar = $2;
}
| ALL TABLES IN_P SCHEMA ColId
{
$$ =
makeNode(PublicationObjSpec);
$$->pubobjtype =
PUBLICATIONOBJ_REL_IN_SCHEMA;
$$->name = $5;
$$->location = @5;
}
| ALL TABLES IN_P SCHEMA CURRENT_SCHEMA /* XXX
should this be "IN_P CURRENT_SCHEMA"? */
  

Re: Column Filtering in Logical Replication

[vignesh C]

On Fri, Sep 24, 2021 at 8:40 AM Amit Kapila  wrote:
>
> On Fri, Sep 24, 2021 at 12:45 AM Tomas Vondra
>  wrote:
> >
> > Hi,
> >
> > I wanted to do a review of this patch, but I'm a bit confused about
> > which patch(es) to review. There's the v5 patch, and then these two
> > patches - which seem to be somewhat duplicate, though.
> >
> > Can anyone explain what's the "current" patch version, or perhaps tell
> > me which of the patches to combine?
> >
>
> I think v5 won't work atop a common grammar patch. There need some
> adjustments in v5. I think it would be good if we can first get the
> common grammar patch reviewed/committed and then build this on top of
> it. The common grammar and the corresponding implementation are being
> accomplished in the Schema support patch, the latest version of which
> is at [1].

I have posted an updated patch with the fixes at [1], please review
the updated patch.
[1] - 
https://www.postgresql.org/message-id/CALDaNm1R-xbQvz4LU5OXu3KKwbWOz3uDcT_YjRU6V0R5FZDYDg%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 22, 2021 at 11:27 AM tanghy.f...@fujitsu.com
 wrote:
>
> On Wednesday, September 22, 2021 11:22 AM Masahiko Sawada 
>  wrote:
> >
> > ---
> > +   if (!IsA(node, String))
> > +   ereport(ERROR,
> > +   
> > errcode(ERRCODE_SYNTAX_ERROR),
> > +   errmsg("invalid schema
> > name at or near"),
> > +
> > parser_errposition(pstate, pubobj->location));
> >
> > The error message should mention where the invalid schema name is at
> > or near. Also, In the following example, the error position in the
> > error message seems not to be where the invalid schemaname s.s is:
> >
> > postgres(1:47707)=# create publication p for all tables in schema s.s;
> > ERROR:  invalid schema name at or near
> > LINE 1: create publication p for all tables in schema s.s;
> >  ^
> >
>
> I noticed this, too. And I think it could be fixed by the following change, 
> thoughts?

I fixed it by updating the location at pubobj_expr

> Besides, about this change in tab-complete.c:
>
> +   else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD|DROP|SET", 
> "SCHEMA"))
> +   COMPLETE_WITH_QUERY(Query_for_list_of_schemas
> +   " UNION SELECT 
> 'CURRENT_SCHEMA'");
>
> It should be "ALL TABLES IN SCHEMA" not "SCHEMA" at the first line, right?

Modified.

The v32 patch attached at [1] handles the above.
[1] - 
https://www.postgresql.org/message-id/CALDaNm1R-xbQvz4LU5OXu3KKwbWOz3uDcT_YjRU6V0R5FZDYDg%40mail.gmail.com

Regards.
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 22, 2021 at 11:31 AM Amit Kapila  wrote:
>
> On Tue, Sep 21, 2021 at 11:39 PM vignesh C  wrote:
> >
> > On Tue, Sep 21, 2021 at 9:03 AM Greg Nancarrow  wrote:
> > >
> > > On Fri, Sep 17, 2021 at 10:09 PM vignesh C  wrote:
> > > >
> > > > Attached v29 patch has the fixes for the same.
> > > >
> > >
> > > Some minor comments on the v29-0002 patch:
> > >
> > > (1)
> > > In get_object_address_publication_schema(), the error message:
> > >
> > > + errmsg("publication tables of schema \"%s\" in publication \"%s\"
> > > does not exist",
> > >
> > > isn't grammatically correct. It should probably be:
> > >
> > > + errmsg("publication tables of schema \"%s\" in publication \"%s\" do
> > > not exist",
> >
> > Modified
> >
>
> I still see the old message in v30. But I have a different suggestion
> for this message. How about changing it to: "publication schema \"%s\"
> in publication \"%s\" does not exist"? This will make it similar to
> other messages and I don't see the need here to add 'tables' as we
> have it in grammar.

Modified

The v32 patch attached at [1] handles the above.
[1] - 
https://www.postgresql.org/message-id/CALDaNm1R-xbQvz4LU5OXu3KKwbWOz3uDcT_YjRU6V0R5FZDYDg%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Tue, Sep 21, 2021 at 6:05 PM Greg Nancarrow  wrote:
>
> On Tue, Sep 21, 2021 at 4:12 PM vignesh C  wrote:
> >
> > > (1)
> > > In get_object_address_publication_schema(), the error message:
> > >
> > > + errmsg("publication tables of schema \"%s\" in publication \"%s\"
> > > does not exist",
> > >
> > > isn't grammatically correct. It should probably be:
> > >
> > > + errmsg("publication tables of schema \"%s\" in publication \"%s\" do
> > > not exist",
> >
> > "does not exist" is used across the file. Should we keep it like that
> > to maintain consistency. Thoughts?
> >
>
> When it's singular, "does not exist" is correct.
> I think currently only this case exists in the publication code.
> e.g.
> "publication \"%s\" does not exist"
> "publication relation \"%s\" in publication \"%s\" does not exist"
>
> But "publication tables" is plural, so it needs to say "do not exist"
> rather than "does not exist".
>
> > >
> > > In the case of "if (!(*nspname))", the following line should probably
> > > be added before returning false:
> > >
> > >*pubname = NULL;
> >
> > In case of failure we return false and don't access it. I felt we
> > could keep it as it is. Thoughts?
> >
>
> OK then, I might be being a bit pedantic.
> (I was just thinking, strictly speaking, we probably shouldn't be
> writing into the caller's pass-by-reference parameters in the case
> false is returned)
>
> > > (5)
> > > I'm wondering, in CheckObjSchemaNotAlreadyInPublication(), instead of
> > > checking "checkobjtype" each loop iteration, wouldn't it be better to
> > > just use the same for-loop in each IF block?
> >
> > I will be changing it to:
> > static void
> > CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
> > PublicationObjSpecType checkobjtype)
> > {
> >   ListCell   *lc;
> >
> >   foreach(lc, rels)
> >   {
> > Relation  rel = (Relation) lfirst(lc);
> > Oid relSchemaId = RelationGetNamespace(rel);
> >
> > if (list_member_oid(schemaidlist, relSchemaId))
> > {
> >   if (checkobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA)
> > ereport(ERROR,
> > errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > errmsg("cannot add schema \"%s\" to publication",
> >  get_namespace_name(relSchemaId)),
> > errdetail("Table \"%s\" in schema \"%s\" is already part
> > of the publication, adding the same schema is not supported.",
> >   RelationGetRelationName(rel),
> >   get_namespace_name(relSchemaId)));
> >   else if (checkobjtype == PUBLICATIONOBJ_TABLE)
> > ereport(ERROR,
> > errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > errmsg("cannot add relation \"%s.%s\" to publication",
> >  get_namespace_name(relSchemaId),
> >  RelationGetRelationName(rel)),
> > errdetail("Table's schema \"%s\" is already part of the
> > publication.",
> >   get_namespace_name(relSchemaId)));
> > }
> >   }
> > }
> > After the change checkobjtype will be checked only once in case of error.
> >
>
> OK.
>
> One thing related to this code is the following:
>
> i)
> postgres=# create publication pub1 for all tables in schema sch1,
> table sch1.test;
> ERROR:  cannot add relation "sch1.test" to publication
> DETAIL:  Table's schema "sch1" is already part of the publication.
>
> ii)
> postgres=# create publication pub1 for table sch1.test, all tables in
> schema sch1;
> ERROR:  cannot add relation "sch1.test" to publication
> DETAIL:  Table's schema "sch1" is already part of the publication.
>
> Notice that in case (ii), the same error message is used, but the
> order of items to be "added" to the publication is the reverse of case
> (i), and really implies the table "sch1.test" was added first, but
> this is not reflected by the error message. So it seems slightly odd
> to say the schema is already part of the publication, when the table
> was actually listed first.
> I'm wondering if this can be improved?
>
> One idea I had was the 

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 22, 2021 at 8:52 AM Masahiko Sawada  wrote:
>
> On Wed, Sep 22, 2021 at 3:02 AM vignesh C  wrote:
> >
> >
> > Attached v30 patch has the fixes for the same.
> >
>
> Thank you for updating the patches.
>
> Here are random comments on v30-0002 patch:
>
> +
> +   if (stmt->action == DEFELEM_SET &&
> !list_length(schemaidlist))
> +   {
> +   delschemas =
> GetPublicationSchemas(pubform->oid);
> +   LockSchemaList(delschemas);
> +   }
>
> I think "list_length(schemaidlist) > 0" would be more readable.

We have refactored the patch which  has removed these changes.

> ---
> This patch introduces some new static functions to publicationcmds.c
> but some have function prototypes but some don't. As far as I checked,
>
> ObjectsInPublicationToOids()
> CheckObjSchemaNotAlreadyInPublication()
> GetAlterPublicationDelRelations()
> AlterPublicationSchemas()
> CheckPublicationAlterTables()
> CheckPublicationAlterSchemas()
> LockSchemaList()
> OpenReliIdList()
> PublicationAddSchemas()
> PublicationDropSchemas()
>
> are newly introduced but only four functions:
>
> OpenReliIdList()
> LockSchemaList()
> PublicationAddSchemas()
> PublicationDropSchemas()
>
> have function prototypes. Actually, there already are functions that
> don't have their prototype in publicationcmds.c. But it seems better
> to clarify what kind of functions don't need to have a prototype at
> least in this file.

My thoughts are the same as that Amit had replied at [1].

> ---
> ISTM we can inline the contents of three functions:
> GetAlterPublicationDelRelations(), CheckPublicationAlterTables(), and
> CheckPublicationAlterSchemas(). These have only one caller and ISTM
> makes the readability worse. I think it's not necessary to make
> functions for them.

We have refactored the patch which  has removed these changes.

> ---
> + * This is dispatcher function for AlterPublicationOptions,
> + * AlterPublicationSchemas and AlterPublicationTables.
>
> As this comment mentioned, AlterPublication() calls
> AlterPublicationTables() and AlterPublicationSchemas() but this
> function also a lot of pre-processing such as building the list and
> some checks, depending on stmt->action before calling these two
> functions. And those two functions also perform some operation
> depending on stmt->action. So ISTM it's better to move those
> pre-processing to these two functions and have AlterPublication() just
> call these two functions. What do you think?

We have refactored the patch which has removed these changes.

> ---
> +List *
> +GetAllSchemasPublicationRelations(Oid puboid, PublicationPartOpt pub_partopt)
>
> Since this function gets all relations in the schema publication, I
> think GetAllSchemaPublicationRelations() would be better as a function
> name (removing 's' before 'P').

Modified

> ---
> +   if (!IsA(node, String))
> +   ereport(ERROR,
> +   errcode(ERRCODE_SYNTAX_ERROR),
> +   errmsg("invalid schema
> name at or near"),
> +
> parser_errposition(pstate, pubobj->location));
>
> The error message should mention where the invalid schema name is at
> or near. Also, In the following example, the error position in the
> error message seems not to be where the invalid schemaname s.s is:

> postgres(1:47707)=# create publication p for all tables in schema s.s;
> ERROR:  invalid schema name at or near
> LINE 1: create publication p for all tables in schema s.s;
>  ^

Modified

> ---
> +   if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE)
> +   {
> +   if (IsA(node, RangeVar))
> +   *rels = lappend(*rels, (RangeVar *) node);
> +   else if (IsA(node, String))
> +   {
> +   RangeVar   *rel = makeRangeVar(NULL,
> strVal(node),
> +
> pubobj->location);
> +
> +   *rels = lappend(*rels, rel);
> +   }
> +   }
> +   else if (pubobj->pubobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA)
> +   {
> (snip)
> +   /* Filter out duplicates if user specifies
> "sch1, sch1" */
> +   *schemas = list_append_unique_oid(*schemas, schemaid);
> +   }
>
> Do we need to filter out duplicates also 

Re: pg_dump does not dump tables created in information_schema schema

[vignesh C]

On Thu, Oct 7, 2021 at 9:30 PM Tom Lane  wrote:
>
> "David G. Johnston"  writes:
> > On Thursday, October 7, 2021, vignesh C  wrote:
> >> Should tables be allowed to create in "information_schema" schema, if
> >> yes should the tables/publications be dumped while dumping database
> >> contents?
>
> > I presume you have to be superuser to do this.  If so, this would seem to
> > fit under the “we don’t stop you, but you shouldn’t” advice that we apply
> > throughout the system, like in say modifying stuff in pg_catalog.
> > Information_schema is an internal schema attached to an static for a given
> > release.
>
> It is (supposed to be) possible for a superuser to drop information_schema
> post-initdb and then recreate it by sourcing the information_schema.sql
> file.  In fact, I seem to recall that we've recommended doing so in past
> minor releases to correct errors in information_schema declarations.
> So it's fairly hard to see how we could enforce prohibitions against
> changing information_schema objects without breaking that use-case.
> On the other hand, just because you did that doesn't mean that you want
> information_schema to start showing up in your dumps.  Quite the opposite
> in fact, because then you'd have problems with trying to load the dump
> into a newer PG version that might need different information_schema
> contents.
>
> So I agree: there's nothing to be done here, and the proposed scenario
> is a case of "superusers should know better than to do that".

Thanks for the clarification.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Oct 11, 2021 at 1:21 PM Greg Nancarrow  wrote:
>
> On Mon, Oct 11, 2021 at 5:39 PM vignesh C  wrote:
> >
> > These comments are fixed in the v38 patch attached.
> >
>
> Thanks for the updates.
> I noticed that these patches don't apply on the latest source (last
> seemed to apply cleanly on HEAD as at about October 6).

I was not able to apply v37 patches, but I was able to apply the v38
version of patches on top of HEAD.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Thu, Oct 21, 2021 at 3:29 PM Greg Nancarrow  wrote:
>
> On Thu, Oct 21, 2021 at 3:25 AM vignesh C  wrote:
> >
> > Attached v44 patch as the fixes for the same.
> >
>
> In the v44-0001 patch, I have some doubts about the condition guarding
> the following code in pg_get_publication_tables():
>
> + if (schemarelids)
> + {
> +/*
> + * If the publication publishes partition changes via their
> + * respective root partitioned tables, we must exclude
> + * partitions in favor of including the root partitioned
> + * tables. Otherwise, the function could return both the child
> + * and parent tables which could cause data of the child table
> + * to be double-published on the subscriber side.
> + *
> + * XXX As of now, we do this when a publication has associated
> + * schema or for all tables publication. See
> + * GetAllTablesPublicationRelations().
> + */
> +tables = list_concat_unique_oid(relids, schemarelids);
> +if (publication->pubviaroot)
> +   tables = filter_partitions(tables);
> + }
>
> Shouldn't a partition be filtered out only if publication->pubviaroot
> and the partition belongs to a schema (i.e. ALL TABLES IN SCHEMA)
> included in the publication?
> The current code seems to filter out partitions of partitioned tables
> included in the publication if ANY schemas are included as part of the
> publication (e.g. which could be a schema not including any
> partitioned tables or partitions).

I could reproduce the issue by using the following test:
--- Setup
create schema sch1;
create schema sch2;
create table sch1.tbl1 (a int) partition by range (a);
create table sch2.tbl1_part1 partition of sch1.tbl1 for values from (1) to (10);
create table sch2.tbl1_part2 partition of sch1.tbl1 for values from
(10) to (20);
create schema sch3;
create table sch3.t1(c1 int);

--- Publication
create publication pub1 for all tables in schema sch3, table
sch1.tbl1, table sch2.tbl1_part1 with ( publish_via_partition_root
=on);
insert into sch1.tbl1 values(1);
insert into sch1.tbl1 values(11);
insert into sch3.t1 values(1);

 Subscription
CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres host=localhost
port=5432' PUBLICATION pub1;

The patch posted at [1] has the fix for the same.
[1] - 
https://www.postgresql.org/message-id/CALDaNm1onqBEr0WE_e7%3DCNw3bURfrGRmbMjX31d-nx3FGLS10A%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Tue, Oct 19, 2021 at 11:23 AM tanghy.f...@fujitsu.com
 wrote:
>
> On Tuesday, October 19, 2021 12:57 PM Amit Kapila  
> wrote:
> >
> > On Tue, Oct 19, 2021 at 9:15 AM tanghy.f...@fujitsu.com
> >  wrote:
> > >
> > > On Monday, October 18, 2021 8:23 PM vignesh C 
> > wrote:
> > > >
> > > > Thanks for the comments, the attached v42 patch has the fixes for the 
> > > > same.
> > >
> > > Thanks for your new patch.
> > >
> > > I tried your patch and found that the permission check for superuser 
> > > didn't work.
> > >
> > > For example:
> > > postgres=# create role r1;
> > > CREATE ROLE
> > > postgres=# grant all privileges on database postgres to r1;
> > > GRANT
> > > postgres=# set role r1;
> > > SET
> > > postgres=> create schema s1;
> > > CREATE SCHEMA
> > > postgres=> create publication pub for all tables in schema s1;
> > > CREATE PUBLICATION
> > >
> > > Role r1 is not superuser, but this role could create publication for all 
> > > tables in
> > schema
> > > successfully, I think it is related the following change. List 
> > > schemaidlist was
> > > not assigned yet. I think we should check it later.
> > >
> >
> > It seems this got broken in yesterday's patch version. Do you think it
> > is a good idea to add a test for this case?
> >
>
> Agreed. Thanks for your suggestion.
>
> I tried to add this test to publication.sql, a patch diff file for this test 
> is attached.

Thanks for the test case, I have merged it to the v43 version patch
attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm2pJ49wAv%3DgEZrAP5%3D_apAzv_rgK3zjX-wfwCY%2BWWfT9w%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Oct 11, 2021 at 7:46 AM tanghy.f...@fujitsu.com
 wrote:
>
> > On Friday, October 8, 2021 7:05 PM Amit Kapila  
> > wrote:
> >
> > v37-0003-Tests-for-FOR-ALL-TABLES-IN-SCHEMA-publication
> > 3.
> > --- a/src/bin/pg_dump/t/002_pg_dump.pl
> > +++ b/src/bin/pg_dump/t/002_pg_dump.pl
> > ..
> > + 'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test' => {
> > + create_order => 51,
> > + create_sql =>
> > +   'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;',
> > + regexp => qr/^
> > + \QALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;\E
> > + /xm,
> > + like   => { %full_runs, section_post_data => 1, },
> > + unlike => { exclude_dump_test_schema => 1, },
> >
> > In this test, won't it exclude the schema dump_test because of unlike?
> > If so, then we don't have coverage for "ALL Tables In Schema" except
> > for public schema?
> >
>
> Yes, the unlike case will exclude the schema dump_test, but I think schema 
> dump_test could be
> dumped in like case.
> I checked the log file src/bin/pg_dump/tmp_check/log/regress_log_002_pg_dump 
> and
> saw some cases were described as "should dump ALTER PUBLICATION pub3 ADD ALL
> TABLES IN SCHEMA dump_test". I think in these cases schema dump_test would be
> dumped.

I agree

> Besides, a small comment on tab-complete.c:
>
> else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL"))
> -   COMPLETE_WITH("TABLES");
> -   else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL", 
> "TABLES")
> -|| Matches("CREATE", "PUBLICATION", MatchAny, "FOR", 
> "TABLE", MatchAny))
> +   COMPLETE_WITH("TABLES", "TABLE IN SCHEMA");
>
>
> COMPLETE_WITH("TABLES", "TABLE IN SCHEMA");
> ->
> COMPLETE_WITH("TABLES", "TABLES IN SCHEMA");

Modified.
This issue is fixed in the v38 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm1TP9S0dif2QWoEUcCtNDop1xJ6Rj1xnu2vS92%3Dj9ahYw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Oct 8, 2021 at 4:34 PM Amit Kapila  wrote:
>
> On Thu, Oct 7, 2021 at 5:19 PM Amit Kapila  wrote:
> >
> > On Wed, Oct 6, 2021 at 11:12 AM vignesh C  wrote:
> > >
> > > Attached v37 patch has the changes for the same.
> > >
> >
> > Few comments:
> > ==
> >
>
> Few more comments:
> 
> v37-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN
> 1.
> + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL",
> "TABLES", "IN", "SCHEMA"))
> + COMPLETE_WITH_QUERY(Query_for_list_of_schemas
> + " UNION SELECT 'CURRENT_SCHEMA' "
> + "UNION SELECT 'WITH ('");
>
> What is the need to display WITH here? It will be displayed after
> Schema name with the below rule:
> + else if (Matches("CREATE", "PUBLICATION", MatchAny,  "FOR", "ALL",
> "TABLES", "IN", "SCHEMA", MatchAny))
> + COMPLETE_WITH("WITH (");

Removed it.

> 2. It seems tab-completion happens incorrectly for the below case:
> create publication pub for all tables in schema s1,
>
> If I press the tab after above, it completes with below which is wrong
> because it will lead to incorrect syntax.
>
> create publication pub for all tables in schema s1, WITH (

Modified

> v37-0003-Tests-for-FOR-ALL-TABLES-IN-SCHEMA-publication
> 3.
> --- a/src/bin/pg_dump/t/002_pg_dump.pl
> +++ b/src/bin/pg_dump/t/002_pg_dump.pl
> ..
> + 'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test' => {
> + create_order => 51,
> + create_sql =>
> +   'ALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;',
> + regexp => qr/^
> + \QALTER PUBLICATION pub3 ADD ALL TABLES IN SCHEMA dump_test;\E
> + /xm,
> + like   => { %full_runs, section_post_data => 1, },
> + unlike => { exclude_dump_test_schema => 1, },
>
> In this test, won't it exclude the schema dump_test because of unlike?
> If so, then we don't have coverage for "ALL Tables In Schema" except
> for public schema?

I noticed that the tap test framework runs pg_dump tests with various
combinations, these tests will be covered in the like tests as Tang
suggested at [1].  In the exclude_dump_test_schema since this sql will
not be present,  exclude_dump_test_schema should be added in the
unlike option, as the validation will fail for exclude-schema option
which will be run with the following command:
pg_dump --no-sync
--file=/home/vignesh/postgres/src/bin/pg_dump/tmp_check/tmp_test_4i8F/exclude_dump_test_schema.sql
--exclude-schema=dump_test postgres

> 4.
> --- a/src/test/regress/expected/publication.out
> +++ b/src/test/regress/expected/publication.out
> ..
> +-- fail - can't add schema to for all tables publication
> +ALTER PUBLICATION testpub_foralltables ADD ALL TABLES IN SCHEMA pub_test;
>
> In the above and all similar comments, it is better to either quote
> 'for all tables' or write in CAPS FOR ALL TABLE or both 'FOR ALL
> TABLE'.

Modified

> 5.
> +\dRp+ testpub1_forschema
> +   Publication testpub1_forschema
> +  Owner   | All tables | Inserts | Updates | Deletes
> | Truncates | Via root
> +--++-+-+-+---+--
> + regress_publication_user | f  | t   | t   | t
> | t | f
> +Tables from schemas:
> +"pub_test1"
> +
> +SELECT p.pubname FROM pg_catalog.pg_publication p,
> pg_catalog.pg_namespace n, pg_catalog.pg_publication_namespace pn
> WHERE n.oid = pn.pnnspid AND p.oid = pn.pnpubid AND n.nspname =
> 'pub_test1' ORDER BY 1;
> +  pubname
> +
> + testpub1_forschema
> +(1 row)
>
> I don't think in the above and similar tests, we need to separately
> check the presence of publication via Select query, if we have tested
> it via psql command. Let's try to keep the meaningful tests.

Removed it.

> 6.
> +INSERT INTO pub_test1.tbl VALUES(1, 'test');
> +-- fail
> +UPDATE pub_test1.tbl SET id = 2;
> +ERROR:  cannot update table "tbl" because it does not have a replica
> identity and publishes updates
> +HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
> +ALTER PUBLICATION testpub1_forschema DROP ALL TABLES IN SCHEMA pub_test1;
> +-- success
> +UPDATE pub_test1.tbl SET id = 2;
> +ALTER PUBLICATION testpub1_forschema SET ALL TABLES IN SCHEMA pub_test1;
> +-- fail
> +UPDATE pub_test1.tbl SET id = 2;
> +ERROR:  cannot update table "tbl" because it does not have a replica
> identity and publishes updates
> +HINT:  To enable u

Re: Column Filtering in Logical Replication

[vignesh C]

On Mon, Sep 27, 2021 at 4:41 PM Amit Kapila  wrote:
>
> On Sat, Sep 25, 2021 at 1:15 PM vignesh C  wrote:
> >
> > On Fri, Sep 24, 2021 at 6:55 PM Alvaro Herrera  
> > wrote:
> > >
> > > On 2021-Sep-23, Amit Kapila wrote:
> > >
> > > > Alvaro, do you have any thoughts on these proposed grammar changes?
> > >
> > > Yeah, I think pubobj_name remains a problem in that you don't know its
> > > return type -- could be a String or a RangeVar, and the user of that
> > > production can't distinguish.  So you're still (unnecessarily, IMV)
> > > stashing an object of undetermined type into ->object.
> > >
> > > I think you should get rid of both pubobj_name and pubobj_expr and do
> > > somethine like this:
> > >
> > > /* FOR TABLE and FOR ALL TABLES IN SCHEMA specifications */
> > > PublicationObjSpec: TABLE ColId
> > > {
> > > $$ = 
> > > makeNode(PublicationObjSpec);
> > > $$->pubobjtype = 
> > > PUBLICATIONOBJ_TABLE;
> > > $$->rangevar = 
> > > makeRangeVarFromQualifiedName($1, NULL, @1, yyscanner);
> > > }
> > > | TABLE ColId indirection
> > > {
> > > $$ = 
> > > makeNode(PublicationObjSpec);
> > > $$->pubobjtype = 
> > > PUBLICATIONOBJ_TABLE;
> > > $$->rangevar = 
> > > makeRangeVarFromQualifiedName($1, $2, @1, yyscanner);
> > > }
> > > | ALL TABLES IN_P SCHEMA ColId
> > > {
> > > $$ = 
> > > makeNode(PublicationObjSpec);
> > > $$->pubobjtype = 
> > > PUBLICATIONOBJ_REL_IN_SCHEMA;
> > > $$->name = $4;
> > > }
> > > | ALL TABLES IN_P SCHEMA CURRENT_SCHEMA /* XXX 
> > > should this be "IN_P CURRENT_SCHEMA"? */
> > > {
> > > $$ = 
> > > makeNode(PublicationObjSpec);
> > > $$->pubobjtype = 
> > > PUBLICATIONOBJ_CURRSCHEMA;
> > > $$->name = $4;
> > > }
> > > | ColId
> > > {
> > > $$ = 
> > > makeNode(PublicationObjSpec);
> > > $$->name = $1;
> > > $$->pubobjtype = 
> > > PUBLICATIONOBJ_CONTINUATION;
> > > }
> > > | ColId indirection
> > > {
> > > $$ = 
> > > makeNode(PublicationObjSpec);
> > > $$->rangevar = 
> > > makeRangeVarFromQualifiedName($1, $2, @1, yyscanner);
> > > $$->pubobjtype = 
> > > PUBLICATIONOBJ_CONTINUATION;
> > > }
> > > | CURRENT_SCHEMA
> > > {
> > > $$ = 
> > > makeNode(PublicationObjSpec);
> > > $$->pubobjtype = 
> > > PUBLICATIONOBJ_CURRSCHEMA;
> > > }
> > > ;
> >
> > Apart from the issue that Hou San pointed, I found one issue with
> > introduction of PUBLICATIONOBJ_CURRSCHEMA, I was not able to
> > differentiate if it is table or schema in the following cases:
> > CREATE PUBLICATION pub1 FOR ALL TABLES IN SCHEMA CURRENT_SCHEMA;
> > CREATE PUBLICATION pub1 FOR ALL

Re: Added schema level support for publication.

[vignesh C]

On Tue, Sep 28, 2021 at 4:35 PM tanghy.f...@fujitsu.com
 wrote:
>
> On Monday, September 27, 2021 1:32 PM, vignesh C  wrote:
>
> >Attached v33 patch has the preprocess_pubobj_list review comment fix
> >suggested by Alvaro at [1]. The
> >v33-0006-Alternate-grammar-for-ALL-TABLES-IN-SCHEMA.patch patch has
> >the grammar changes as suggested by Alvaro at [1]. If we agree this is
> >better, I will merge this into the 0001 patch.
> >[1] - 
> >https://www.postgresql.org/message-id/202109241325.eag5g6mpvoup%40alvherre.pgsql
>
> About the schema patch, I think a schema and a table which belongs to this 
> schema shouldn't be specified at the same time.
> But what if someone uses "ALTER TABLE ... SET SCHEMA ..." after "CREATE 
> PUBLICATION"?
>
> For example:
>
> create schema sch1;
> create schema sch2;
> create table sch2.t (a int);
> create publication pub1 for all tables in schema sch1, table sch2.t; alter 
> table sch2.t set schema sch1;
>
> postgres=# \dRp+
>   Publication pub1
>   Owner   | All tables | Inserts | Updates | Deletes | Truncates | Via root
> --++-+-+-+---+--
> --++-+-+-+---+
>  postgres | f  | t   | t   | t   | t | f
> Tables:
> "sch1.t"
> Tables from schemas:
> "sch1"
>
> Table t has been output twice.
> I think this should not be supported, should we do something for this 
> scenario?

Yes this should not be supported, we should throw an error in this case.
This is handled in the v34 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm2Z9TfuoCf09YGKfwy7F1NwC4iCXJGTaZS%3DchH6VHtadQ%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Sep 27, 2021 at 4:51 PM Greg Nancarrow  wrote:
>
> On Mon, Sep 27, 2021 at 2:32 PM vignesh C  wrote:
> >
> > Attached v33 patch has the preprocess_pubobj_list review comment fix
> > suggested by Alvaro at [1].
>
> In the v33-0003 patch, there's a couple of error-case tests that have
> comments copied from success-case tests:
>
> +-- should be able to add table to schema publication
> ...
> +-- should be able to drop table from schema publication
> ...
>
> These should be changed to something similar to that used for other
> error-case tests, like:
>
> +-- fail - can't add a table of the same schema to the schema publication
> +-- fail - can't drop a table from the schema publication which isn't
> in the publication

Modified

> Also, for the following error:
>
> ERROR:  cannot add ... to publication
> DETAIL:  Table's schema "" is already part of the publication
> or part of the specified schema list.
>
> there needs to be a test case to test the "... or part of the
> specified schema list" case.

Added the test
This is handled in the v34 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm2Z9TfuoCf09YGKfwy7F1NwC4iCXJGTaZS%3DchH6VHtadQ%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Mon, Sep 27, 2021 at 2:46 PM Greg Nancarrow  wrote:
>
> On Mon, Sep 27, 2021 at 2:32 PM vignesh C  wrote:
> >
> > Attached v33 patch has the preprocess_pubobj_list review comment fix
> > suggested by Alvaro at [1].
>
> A minor point I noticed in the v33-0002 patch, in the code added to
> the listSchemas() function of src/bin/psql/describe.c, shouldn't it
> "return false" (not true) if PSQLexec() fails?
> Also, since the PQExpBufferData buf is re-used in the added code, it's
> handling is a little inconsistent to similar existing code.
> See below for suggested update.

Modified
This is handled in the v34 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm2Z9TfuoCf09YGKfwy7F1NwC4iCXJGTaZS%3DchH6VHtadQ%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 29, 2021 at 10:46 AM Amit Kapila  wrote:
>
> On Tue, Sep 28, 2021 at 8:15 PM vignesh C  wrote:
> >
> > On Mon, Sep 27, 2021 at 12:15 PM houzj.f...@fujitsu.com
> >  wrote:
> >
> > Attached v34 patch has the changes for the same.
> >
>
> Few comments on v34-0001-Added-schema-level-support-for-publication
> ==
> 1.
> + * This rule parses publication object with and without keyword prefix.
>
> I think we should write it as: "This rule parses publication objects
> with and without keyword prefixes."

Modified

> 2.
> + * For the object without keyword prefix, we cannot just use
> relation_expr here,
> + * because some extended expression in relation_expr cannot be used as a
>
> /expression/expressions

Modified

> 3.
> +/*
> + * Process pubobjspec_list to check for errors in any of the objects and
> + * convert PUBLICATIONOBJ_CONTINUATION into appropriate 
> PublicationObjSpecType
> + * type.

Modified to remove type as discussed offline.

> 4.
> + /*
> + * Check if setting the relation to a different schema will result in the
> + * publication having schema and same schema's table in the publication.
> + */
> + if (stmt->objectType == OBJECT_TABLE)
> + {
> + ListCell   *lc;
> + List*schemaPubids = GetSchemaPublications(nspOid);
> + foreach(lc, schemaPubids)
> + {
> + Oid pubid = lfirst_oid(lc);
> + if (list_member_oid(GetPublicationRelations(pubid, PUBLICATION_PART_ALL),
> + relid))
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot move table \"%s\" to schema \"%s\"",
> +RelationGetRelationName(rel), stmt->newschema),
> + errdetail("Altering table will result in having schema \"%s\" and
> same schema's table \"%s\" in the publication \"%s\" which is not
> supported.",
> +   stmt->newschema,
> +   RelationGetRelationName(rel),
> +   get_publication_name(pubid, false)));
> + }
> + }
>
> Let's slightly change the comment as: "Check that setting the relation
> to a different schema won't result in the publication having schema
> and the same schema's table." and errdetail as: "The schema \"%s\" and
> same schema's table \"%s\" cannot be part of the same publication
> \"%s\"."
>
> Maybe it is better to specify that this will disallow the partition table 
> case.

Modified, I did not add the above as we are allowing it.

> 5.
> ObjectsInPublicationToOids()
> {
> ..
> + pubobj = (PublicationObjSpec *) lfirst(cell);
> + if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE)
>
> It is better to keep an empty line between above two lines.

Modified

> 6.
> List*schemaPubids = GetSchemaPublications(nspOid);
> foreach(lc, schemaPubids)
> ..
> Oid pubid = lfirst_oid(lc);
> if (list_member_oid(GetPublicationRelations(pubid, PUBLICATION_PART_ALL),
>
> Add an empty line between each of the above two lines.

Modified

> 7.
> + /*
> + * Schema lock is held until the publication is altered to prevent
> + * concurrent schema deletion. No need to unlock the schemas, the locks
> + * will be released automatically at the end of alter publication command.
> + */
> + LockSchemaList(schemaidlist);
>
> I think it is better to add a similar comment at other places where
> this function is called. And we can shorten the comment atop
> LockSchemaList to something like: "The schemas specified in the schema
> list are locked in AccessShareLock mode in order to prevent concurrent
> schema deletion."

Modified

> 8. In CreatePublication(), the check if (stmt->for_all_tables) can be
> the first check and then in else if we can process tables and schemas.

Modified

> 9.
> AlterPublication()
> {
> ..
> + /* Lock the publication so nobody else can do anything with it. */
> + LockDatabaseObject(PublicationRelationId, pubform->oid, 0,
> +AccessExclusiveLock);
>
> I think it is better to say why we need this lock. So, can we change
> the comment to something like: "Lock the publication so nobody else
> can do anything with it. This prevents concurrent alter to add
> table(s) that were already going to become part of the publication by
> adding corresponding schema(s) via this command and similarly it will
> prevent the concurrent addition of schema(s) for which there is any
> corresponding table being added by this command."

Modified

These comments are handled in the v35 version patch attached at [1]
[1] - 
https://www.postgresql.org/message-id/CALDaNm2yJOEPCqR%3DgTMEwveJujH9c9_z4LhKmk2T3vZH7T1DLQ%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 29, 2021 at 11:49 AM Greg Nancarrow  wrote:
>
> On Wed, Sep 29, 2021 at 3:16 PM Amit Kapila  wrote:
> >
> > 4.
> > + /*
> > + * Check if setting the relation to a different schema will result in the
> > + * publication having schema and same schema's table in the publication.
> > + */
> > + if (stmt->objectType == OBJECT_TABLE)
> > + {
> > + ListCell   *lc;
> > + List*schemaPubids = GetSchemaPublications(nspOid);
> > + foreach(lc, schemaPubids)
> > + {
> > + Oid pubid = lfirst_oid(lc);
> > + if (list_member_oid(GetPublicationRelations(pubid, PUBLICATION_PART_ALL),
> > + relid))
> > + ereport(ERROR,
> > + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > + errmsg("cannot move table \"%s\" to schema \"%s\"",
> > +RelationGetRelationName(rel), stmt->newschema),
> > + errdetail("Altering table will result in having schema \"%s\" and
> > same schema's table \"%s\" in the publication \"%s\" which is not
> > supported.",
> > +   stmt->newschema,
> > +   RelationGetRelationName(rel),
> > +   get_publication_name(pubid, false)));
> > + }
> > + }
> >
> > Let's slightly change the comment as: "Check that setting the relation
> > to a different schema won't result in the publication having schema
> > and the same schema's table." and errdetail as: "The schema \"%s\" and
> > same schema's table \"%s\" cannot be part of the same publication
> > \"%s\"."
> >
>
> Since this code is in AlterTableNamespace() and the relation being
> checked may or may not be part of a publication, I'd use "a
> publication" instead of "the publication" in the comment.
> Also, I'd say that we're doing the check because the mentioned
> combination is not supported.
>
> i.e. "Check that setting the relation to a different schema won't
> result in a publication having both a schema and the same schema's
> table, as this is not supported."

Thanks for the comment, I have handled it in the v35 version patch
attached at [1]
[1] - 
https://www.postgresql.org/message-id/CALDaNm2yJOEPCqR%3DgTMEwveJujH9c9_z4LhKmk2T3vZH7T1DLQ%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Sep 29, 2021 at 9:07 AM houzj.f...@fujitsu.com
 wrote:
>
> On Tues, Sep 28, 2021 10:46 PM vignesh C  wrote:
> > Attached v34 patch has the changes for the same.
>
> Thanks for updating the patch.
> Here are a few comments.
>
> 1)
> + * ALL TABLES IN SCHEMA schema [[, ...]
>
> [[ -> [

Modified

> 2)
> +   /* ALTER PUBLICATION ... ADD/DROP TABLE/ALL TABLES IN SCHEMA 
> parameters */
>
> The two '/' seems a bit unclear and it doesn't mention the SET case.
> Maybe we can write like:
>
> /* parameters used for ALTER PUBLICATION ... ADD/DROP/SET publication objects 
> */

Modified

> 3)
> +   /*
> +* Check if setting the relation to a different schema will result in 
> the
> +* publication having schema and same schema's table in the 
> publication.
> +*/
> +   if (stmt->objectType == OBJECT_TABLE)
> +   {
> +   ListCell   *lc;
> +   List   *schemaPubids = GetSchemaPublications(nspOid);
> +   foreach(lc, schemaPubids)
> +   {
> +   Oid pubid = lfirst_oid(lc);
> +   if (list_member_oid(GetPublicationRelations(pubid, 
> PUBLICATION_PART_ALL),
> +   relid))
> +   ereport(ERROR,
>
> How about we check this case like the following ?
>
> List   *schemaPubids = GetSchemaPublications(nspOid);
> List   *relPubids = GetRelationPublications(RelationGetRelid(rel));
> if (list_intersection(schemaPubids, relPubids))
> ereport(ERROR, ...

Modified it with slight changes without using list_intersection.

These comments are handled in the v35 version patch attached at [1]
[1] - 
https://www.postgresql.org/message-id/CALDaNm2yJOEPCqR%3DgTMEwveJujH9c9_z4LhKmk2T3vZH7T1DLQ%40mail.gmail.com

Regards,
VIgnesh

pg_dump does not dump tables created in information_schema schema

[vignesh C]

Hi,

I was able to create a table in "information_schema" schema, but
pg_dump does not dumps the table that was created in
"information_schema" schema:
create table information_schema.t1(c1 int);

The similar problem exists in case of create publication, we are able
to create publications for tables present in "information_schema"
schema, but pg_dump does not dump the publication to include the
information_schema.t1 information.
create publication pub1 for table information_schema.t1;

Should tables be allowed to create in "information_schema" schema, if
yes should the tables/publications be dumped while dumping database
contents?
Thoughts?

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Tue, Sep 21, 2021 at 9:03 AM Greg Nancarrow  wrote:
>
> On Fri, Sep 17, 2021 at 10:09 PM vignesh C  wrote:
> >
> > Attached v29 patch has the fixes for the same.
> >
>
> Some minor comments on the v29-0002 patch:
>
> (1)
> In get_object_address_publication_schema(), the error message:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\"
> does not exist",
>
> isn't grammatically correct. It should probably be:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\" do
> not exist",

"does not exist" is used across the file. Should we keep it like that
to maintain consistency. Thoughts?

> (2)
> getPublicationSchemaInfo() function header.
> "string" should be "strings"
>
> BEFORE:
> + * nspname. Both pubname and nspname are palloc'd string which will be freed 
> by
> AFTER:
> + * nspname. Both pubname and nspname are palloc'd strings which will
> be freed by

I will change it in the next version.

> (3)
> getPublicationSchemaInfo()
>
> In the case of "if (!(*nspname))", the following line should probably
> be added before returning false:
>
>*pubname = NULL;

In case of failure we return false and don't access it. I felt we
could keep it as it is. Thoughts?

> (4)
> GetAllSchemasPublicationRelations() function header
>
> Shouldn't:
>
> + * Gets the list of all relations published by FOR ALL TABLES IN SCHEMA
> + * publication(s).
>
> actually say:
>
> + * Gets the list of all relations published by a FOR ALL TABLES IN SCHEMA
> + * publication.
>
> since it is for a specified publication?

I will change it in the next version.

> (5)
> I'm wondering, in CheckObjSchemaNotAlreadyInPublication(), instead of
> checking "checkobjtype" each loop iteration, wouldn't it be better to
> just use the same for-loop in each IF block?

I will be changing it to:
static void
CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
PublicationObjSpecType checkobjtype)
{
  ListCell   *lc;

  foreach(lc, rels)
  {
Relation  rel = (Relation) lfirst(lc);
Oid relSchemaId = RelationGetNamespace(rel);

if (list_member_oid(schemaidlist, relSchemaId))
{
  if (checkobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("cannot add schema \"%s\" to publication",
 get_namespace_name(relSchemaId)),
errdetail("Table \"%s\" in schema \"%s\" is already part
of the publication, adding the same schema is not supported.",
  RelationGetRelationName(rel),
  get_namespace_name(relSchemaId)));
  else if (checkobjtype == PUBLICATIONOBJ_TABLE)
ereport(ERROR,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("cannot add relation \"%s.%s\" to publication",
 get_namespace_name(relSchemaId),
 RelationGetRelationName(rel)),
errdetail("Table's schema \"%s\" is already part of the
publication.",
  get_namespace_name(relSchemaId)));
}
  }
}
After the change checkobjtype will be checked only once in case of error.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Tue, Oct 5, 2021 at 4:41 PM Amit Kapila  wrote:
>
> On Tue, Oct 5, 2021 at 11:10 AM Greg Nancarrow  wrote:
> >
> > On Tue, Oct 5, 2021 at 3:11 PM Amit Kapila  wrote:
> > >
> > > > Code has been added to prevent a table being set (via ALTER TABLE) to
> > > > UNLOGGED if it is part of a publication, but I found that I could
> > > > still add all tables of a schema having a table that is UNLOGGED:
> > > >
> > > > postgres=# create schema sch;
> > > > CREATE SCHEMA
> > > > postgres=# create unlogged table sch.test(i int);
> > > > CREATE TABLE
> > > > postgres=# create publication pub for table sch.test;
> > > > ERROR:  cannot add relation "test" to publication
> > > > DETAIL:  Temporary and unlogged relations cannot be replicated.
> > > > postgres=# create publication pub for all tables in schema sch;
> > > > CREATE PUBLICATION
> > > >
> > >
> > > What about when you use "create publication pub for all tables;"? I
> > > think that also works, now on similar lines shouldn't the behavior of
> > > "all tables in schema" publication be the same? I mean if we want we
> > > can detect and give an error but is it advisable to give an error if
> > > there are just one or few tables in schema that are unlogged?
> > >
> >
> >
> ..
> > Yes, I agree that ALL TABLES IN SCHEMA should behave the same as the
> > ALL TABLES case.
> > Problem is, shouldn't setting UNLOGGED on a table only then be
> > disallowed if that table was publicised using FOR TABLE?
> >
>
> Right, I also think so. Let us see what Vignesh or others think on this 
> matter.

Even I felt ALL TABLES IN SCHEMA should behave the same way as the ALL
TABLES case. I will keep the create publication behavior as it is i.e.
to allow even if unlogged tables are present and change the below
alter table behavior which was throwing error to be successful to keep
it similar to ALL TABLES publication:
alter table sch.test3 set unlogged;
ERROR:  cannot change table "test3" to unlogged because it is part of
a publication
DETAIL:  Unlogged relations cannot be replicated.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Tue, Oct 5, 2021 at 6:57 AM Greg Nancarrow  wrote:
>
> On Mon, Oct 4, 2021 at 4:55 AM vignesh C  wrote:
> >
> > Attached v36 patch has the changes for the same.
> >
>
> I have some comments on the v36-0001 patch:
>
> src/backend/commands/publicationcmds.c
>
> (1)
> GetPublicationSchemas()
>
> + /* Find all publications associated with the schema */
> + pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
>
> I think the comment is not correct. It should say:
>
> + /* Find all schemas associated with the publication */

Modified

> (2)
> AlterPublicationSchemas
>
> I think that a comment should be added for the following lines,
> something like the comment used in the similar check in
> AlterPublicationTables():
>
> + if (!schemaidlist && stmt->action != DEFELEM_SET)
> + return;

Modified

> (3)
> CheckAlterPublication
>
> Minor comment fix suggested:
>
> BEFORE:
> + * Check if relations and schemas can be in given publication and throws
> AFTER:
> + * Check if relations and schemas can be in a given publication and throw

Modified

> (4)
> LockSchemaList()
>
> Suggest re-word of comment, to match imperative comment style used
> elsewhere in this code.
>
> BEFORE:
> + * The schemas specified in the schema list are locked in AccessShareLock 
> mode
> AFTER:
> + * Lock the schemas specified in the schema list in AccessShareLock mode

Modified

>
> src/backend/commands/tablecmds.c
>
> (5)
>
> Code has been added to prevent a table being set (via ALTER TABLE) to
> UNLOGGED if it is part of a publication, but I found that I could
> still add all tables of a schema having a table that is UNLOGGED:
>
> postgres=# create schema sch;
> CREATE SCHEMA
> postgres=# create unlogged table sch.test(i int);
> CREATE TABLE
> postgres=# create publication pub for table sch.test;
> ERROR:  cannot add relation "test" to publication
> DETAIL:  Temporary and unlogged relations cannot be replicated.
> postgres=# create publication pub for all tables in schema sch;
> CREATE PUBLICATION

I have changed the alter table behavior to allow setting it to an
unlogged table to keep the behavior similar to "ALL TABLES"
publication. I have kept the create publication behavior as it is, it
will be similar to "ALL TABLES" publication i.e to allow create
publication even if there are unlogged tables present.

These comments are handled in the v37 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm0ON%3D012jGC3oquSVVWTWXhHG0q8yOyRROVGFR9PjWa-g%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Thu, Sep 23, 2021 at 12:32 PM houzj.f...@fujitsu.com
 wrote:
>
> On Thur, Sep 23, 2021 11:06 AM Greg Nancarrow  wrote:
> > On Wed, Sep 22, 2021 at 9:33 PM houzj.f...@fujitsu.com 
> >  wrote:
> > >
> > > >
> > > > How do you suggest changing it?
> > >
> > > Personally, I think we'd better move the code about changing
> > > publication's tablelist into AlterPublicationTables and the code about
> > > changing publication's schemalist into AlterPublicationSchemas. It's
> > > similar to what the v29-patchset did, the difference is the SET
> > > action, I suggest we drop all the tables in function
> > > AlterPublicationTables when user only set schemas and drop all the
> > > schema in AlterPublicationSchemas when user only set tables. In this
> > > approach, we can keep schema and relation code separate and don't need to
> > worry about the locking order.
> > >
> > > Attach a top-up patch which refactor the code like above.
> > > Thoughts ?
> > >
> >
> > Sounds like a good idea.
> > Is it possible to incorporate the existing
> > CheckPublicationAlterTables() and CheckPublicationAlterSchemas() functions
> > into your suggested update?
> > I think it might tidy up the error-checking a bit.
>
> I agreed we can put the check about ALL TABLE and superuser into a function
> like what the v30-patchset did. But I have some hesitations about the code
> related to CheckObjSchemaNotAlreadyInPublication(). Currently, we need to open
> and lock the table before invoking the CheckObjxxx function, ISTM we'd better
> open the table in function AlterPublicationTables. Maybe we can wait for the
> author's(Vignesh) opinion.

I felt keeping the code related to
CheckObjSchemaNotAlreadyInPublication as it is in
AlterPublicationTables and AlterPublicationSchemas is better.

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Thu, Sep 23, 2021 at 12:22 PM houzj.f...@fujitsu.com
 wrote:
>
> From Thurs, Sep 23, 2021 12:09 PM Amit Kapila  wrote:
> > On Wed, Sep 22, 2021 at 5:03 PM Hou Zhijie  wrote:
> > >
> > > Personally, I think we'd better move the code about changing publication's
> > > tablelist into AlterPublicationTables and the code about changing
> > publication's
> > > schemalist into AlterPublicationSchemas. It's similar to what the
> > v29-patchset
> > > did, the difference is the SET action, I suggest we drop all the tables in
> > > function AlterPublicationTables when user only set schemas and drop all 
> > > the
> > > schema in AlterPublicationSchemas when user only set tables. In this
> > approach,
> > > we can keep schema and relation code separate and don't need to worry
> > > about the locking order.
> > >
> > > Attach a top-up patch which refactor the code like above.
> > >
> >
> > Good suggestion. I think it would still be better if we can move the
> > checks related to superuser and puballtables into a separate function
> > that gets called before taking a lock on publication.
>
> I agreed.
>
> I noticed v30-0001 has been committed with some minor changes, and the 
> V30-0002
> patchset need to be rebased accordingly. Attach a rebased version patch set to
> make cfbot happy. Also Attach the two top-up patches which refactor the code 
> as
> suggested. (top-up patch 1 is to keep schema and table code separate, top-up
> patch 2 is to move some cheap check into a function and invoke it before
> locking.)

Thanks for the patches, the changes simplifies alterpublications code
and handles the drop object in a better way. I have merged it to 0001
patch at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm1R-xbQvz4LU5OXu3KKwbWOz3uDcT_YjRU6V0R5FZDYDg%40mail.gmail.com

Regards,
Vignesh

Added missing invalidations for all tables publication

[vignesh C]

Hi,

Relation invalidation was missing in case of create publication and
drop publication of "FOR ALL TABLES" publication, added so that the
publication information can be rebuilt. Without these invalidation
update/delete operations on the relation will be successful in the
publisher which will later result in conflict in the subscriber.
Thanks to Amit for identifying the issue at [1]. Attached patch has
the fix for the same.
Thoughts?

[1] - 
https://www.postgresql.org/message-id/CAA4eK1LtTXMqu-UbcByjHw%2BaKP38t4%2Br7kyKnmBQMA-__9U52A%40mail.gmail.com

Regards,
Vignesh
From fd58e547c22723856a2f18c306b17ab6e59cddb1 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Mon, 30 Aug 2021 22:29:07 +0530
Subject: [PATCH v1] Added missing invalidations for all tables publication.

Relation invalidation was missing in case of create publication and drop
publication of "FOR ALL TABLES" publication, added them so that the
publication information can be rebuilt.
---
 src/backend/catalog/dependency.c  |  5 +++-
 src/backend/commands/publicationcmds.c| 33 +++
 src/include/commands/publicationcmds.h|  1 +
 src/test/regress/expected/publication.out | 13 +
 src/test/regress/sql/publication.sql  | 12 +
 5 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 76b65e39c4..91c3e976e0 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1460,6 +1460,10 @@ doDeletion(const ObjectAddress *object, int flags)
 			RemovePublicationRelById(object->objectId);
 			break;
 
+		case OCLASS_PUBLICATION:
+			RemovePublicationById(object->objectId);
+			break;
+
 		case OCLASS_CAST:
 		case OCLASS_COLLATION:
 		case OCLASS_CONVERSION:
@@ -1478,7 +1482,6 @@ doDeletion(const ObjectAddress *object, int flags)
 		case OCLASS_USER_MAPPING:
 		case OCLASS_DEFACL:
 		case OCLASS_EVENT_TRIGGER:
-		case OCLASS_PUBLICATION:
 		case OCLASS_TRANSFORM:
 			DropObjectById(object);
 			break;
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 8487eeb7e6..54eb0b20b4 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -234,6 +234,9 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
 		PublicationAddTables(puboid, rels, true, NULL);
 		CloseTableList(rels);
 	}
+	/* Invalidate relcache so that publication info is rebuilt. */
+	else if (stmt->for_all_tables)
+		CacheInvalidateRelcacheAll();
 
 	table_close(rel, RowExclusiveLock);
 
@@ -497,6 +500,36 @@ RemovePublicationRelById(Oid proid)
 	table_close(rel, RowExclusiveLock);
 }
 
+/*
+ * Remove the publication by mapping OID.
+ */
+void
+RemovePublicationById(Oid pubid)
+{
+	Relation	rel;
+	HeapTuple	tup;
+	Form_pg_publication pubform;
+
+	rel = table_open(PublicationRelationId, RowExclusiveLock);
+
+	tup = SearchSysCache1(PUBLICATIONOID, ObjectIdGetDatum(pubid));
+	if (!HeapTupleIsValid(tup))
+		elog(ERROR, "cache lookup failed for publication %u",
+			 pubid);
+
+	pubform = (Form_pg_publication) GETSTRUCT(tup);
+
+	/* Invalidate relcache so that publication info is rebuilt. */
+	if (pubform->puballtables)
+		CacheInvalidateRelcacheAll();
+
+	CatalogTupleDelete(rel, >t_self);
+
+	ReleaseSysCache(tup);
+
+	table_close(rel, RowExclusiveLock);
+}
+
 /*
  * Open relations specified by a RangeVar list.
  * The returned tables are locked in ShareUpdateExclusiveLock mode in order to
diff --git a/src/include/commands/publicationcmds.h b/src/include/commands/publicationcmds.h
index a3fa2ac6cd..c98d519b29 100644
--- a/src/include/commands/publicationcmds.h
+++ b/src/include/commands/publicationcmds.h
@@ -20,6 +20,7 @@
 
 extern ObjectAddress CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt);
 extern void AlterPublication(ParseState *pstate, AlterPublicationStmt *stmt);
+extern void RemovePublicationById(Oid pubid);
 extern void RemovePublicationRelById(Oid proid);
 
 extern ObjectAddress AlterPublicationOwner(const char *name, Oid newOwnerId);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 4a5ef0bc24..c8dc33f872 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -158,6 +158,19 @@ Tables:
 
 DROP TABLE testpub_parted1;
 DROP PUBLICATION testpub_forparted, testpub_forparted1;
+-- Test cache invalidation FOR ALL TABLES publication
+SET client_min_messages = 'ERROR';
+CREATE TABLE testpub_tbl4(a int);
+CREATE PUBLICATION testpub_foralltables FOR ALL TABLES;
+RESET client_min_messages;
+-- fail missing REPLICA IDENTITY
+UPDATE testpub_tbl4 set a = 2;
+ERROR:  cannot update table "testpub_tbl4" because it does not have a replica identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+DROP PUBLICATI