Re: Added schema level support for publication.

[vignesh C]

On Fri, Dec 10, 2021 at 6:24 AM Alvaro Herrera  wrote:
>
> I just noticed that this (commit 5a2832465fd8) added a separate catalog
> to store schemas which are part of a publication, side-by-side with the
> catalog to store relations which are part of a publication.  This seems
> a strange way to represent publication membership: in order to find out
> what objects are members of a publication, you have to scan both
> pg_publication_rel and pg_publication_namespace.  Wouldn't it make more
> sense to have a single catalog for both things, maybe something like
>
> pg_publication_object
> oid OID -- unique key (for pg_depend)
> prpubid OID -- of pg_publication
> prrelid OID -- OID of relation, or 0 if not a relation
> prnspid OID -- OID of namespace, or 0 if not a namespace
>
> which seems more natural to me, and pollutes the system less with weird
> syscaches, etc.
>
> What do you think?

I felt the existing tables are better normalized than the proposed one.

Regards,
Vignesh

Re: Added schema level support for publication.

[Amit Kapila]

On Fri, Dec 10, 2021 at 6:24 AM Alvaro Herrera  wrote:
>
> I just noticed that this (commit 5a2832465fd8) added a separate catalog
> to store schemas which are part of a publication, side-by-side with the
> catalog to store relations which are part of a publication.  This seems
> a strange way to represent publication membership: in order to find out
> what objects are members of a publication, you have to scan both
> pg_publication_rel and pg_publication_namespace.  Wouldn't it make more
> sense to have a single catalog for both things, maybe something like
>
> pg_publication_object
> oid OID -- unique key (for pg_depend)
> prpubid OID -- of pg_publication
> prrelid OID -- OID of relation, or 0 if not a relation
> prnspid OID -- OID of namespace, or 0 if not a namespace
>
> which seems more natural to me, and pollutes the system less with weird
> syscaches, etc.
>

It will unnecessarily increase the size per row for FOR TABLE
publication both because of the additional column and additional index
(schema_id, pub_id) and vice versa. I think future projects (like
row_filter, column_filter, etc) will make this impact much bigger as
new columns for those won't be required for schema publications.
Basically, as soon as we start to store additional properties for
different objects, storing different objects together would start
becoming more and more worse. This will also in turn increase the scan
cost where we need only schema or rel publication access like where
ever we are using PUBLICATIONNAMESPACEMAP. I think it will increase
the cost of scanning table publications as its corresponding index
will be larger.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Alvaro Herrera]

I just noticed that this (commit 5a2832465fd8) added a separate catalog
to store schemas which are part of a publication, side-by-side with the
catalog to store relations which are part of a publication.  This seems
a strange way to represent publication membership: in order to find out
what objects are members of a publication, you have to scan both
pg_publication_rel and pg_publication_namespace.  Wouldn't it make more
sense to have a single catalog for both things, maybe something like

pg_publication_object
oid OID -- unique key (for pg_depend)
prpubid OID -- of pg_publication
prrelid OID -- OID of relation, or 0 if not a relation
prnspid OID -- OID of namespace, or 0 if not a namespace

which seems more natural to me, and pollutes the system less with weird
syscaches, etc.

What do you think?

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Re: Added schema level support for publication.

[vignesh C]

On Tue, Nov 9, 2021 at 2:51 PM Amit Kapila  wrote:
>
> On Tue, Nov 9, 2021 at 7:20 AM Peter Smith  wrote:
> >
> > FYI -  I spotted a trivial SQL mistake (?) of the schema publication patch 
> > [1].
> >
> > See the file describe.c, function describeOneTableDetails.
> > The new SQL has a 3rd UNION that looks like:
> >
> > ...
> > "UNION\n"
> > "SELECT pubname\n"
> > "FROM pg_catalog.pg_publication p\n"
> > "WHERE puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
> > "ORDER BY 1;",
> > oid, oid, oid, oid);
> >
> > Notice that there is a table alias "p" but it is never used. It seems
> > to me like it is just an accidental omission. I think it should be
> > written like -
> >
> > BEFORE:
> > "WHERE puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
> > AFTER:
> > "WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
> >
> > Doing this will make it consistent with the SQL of the nearby "else"
> > case which uses the same alias as expected.
> >
>
> The above makes sense to me. So, pushed a fix for this along with
> Vignesh's patch to fix other comments related to this work.

Thanks for committing the patch. I have changed the status of the
Commitfest entry for this patch to Committed. Adding of the view can
be handled once the Sequence patch is committed.

Regards,
Vignesh

Re: Added schema level support for publication.

[Amit Kapila]

On Tue, Nov 9, 2021 at 7:20 AM Peter Smith  wrote:
>
> FYI -  I spotted a trivial SQL mistake (?) of the schema publication patch 
> [1].
>
> See the file describe.c, function describeOneTableDetails.
> The new SQL has a 3rd UNION that looks like:
>
> ...
> "UNION\n"
> "SELECT pubname\n"
> "FROM pg_catalog.pg_publication p\n"
> "WHERE puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
> "ORDER BY 1;",
> oid, oid, oid, oid);
>
> Notice that there is a table alias "p" but it is never used. It seems
> to me like it is just an accidental omission. I think it should be
> written like -
>
> BEFORE:
> "WHERE puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
> AFTER:
> "WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
>
> Doing this will make it consistent with the SQL of the nearby "else"
> case which uses the same alias as expected.
>

The above makes sense to me. So, pushed a fix for this along with
Vignesh's patch to fix other comments related to this work.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Peter Smith]

FYI -  I spotted a trivial SQL mistake (?) of the schema publication patch [1].

See the file describe.c, function describeOneTableDetails.
The new SQL has a 3rd UNION that looks like:

...
"UNION\n"
"SELECT pubname\n"
"FROM pg_catalog.pg_publication p\n"
"WHERE puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
"ORDER BY 1;",
oid, oid, oid, oid);

Notice that there is a table alias "p" but it is never used. It seems
to me like it is just an accidental omission. I think it should be
written like -

BEFORE:
"WHERE puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
AFTER:
"WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"

Doing this will make it consistent with the SQL of the nearby "else"
case which uses the same alias as expected.

--
[1] 
https://github.com/postgres/postgres/commit/5a2832465fd8984d089e8c44c094e6900d987fcd

Kind Regards,
Peter Smith.
Fujitsu Australia

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

> On Thu, Nov 4, 2021 at 3:24 PM vignesh C  wrote:
> >
> > On Thu, Nov 4, 2021 at 5:41 AM Peter Smith 
> wrote:
> > >
> > > FYI - I found a small problem with one of the new PublicationObjSpec
> > > parser error messages that was introduced by the recent schema
> > > publication commit [1].
> > >
> > > The error message text is assuming that the error originates from
> > > CREATE PUBLICATION, but actually that same error can also come from
> > > ALTER PUBLICATION.
> > >
> > > For example,
> > >
> > > e.g.1) Here the error came from CREATE PUBLICATION, so the message
> > > text looks OK.
> > >
> > > test_pub=# CREATE PUBLICATION p1 FOR t1;
> > > 2021-11-04 10:50:17.208 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES
> > > IN SCHEMA should be specified before the table/schema name(s) at
> > > character 27
> > > 2021-11-04 10:50:17.208 AEDT [738] STATEMENT:  CREATE PUBLICATION p1
> > > FOR t1;
> > > ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified
> > > before the table/schema name(s) LINE 1: CREATE PUBLICATION p1 FOR
> > > t1;
> > >   ^
> > > ~~
> > >
> > > e.g.2) Here the error came from ALTER PUBLICATION, so the message
> > > text is not OK because the ALTER syntax [2] does not even have a FOR
> > > keyword.
> > >
> > > test_pub=# ALTER PUBLICATION p1 SET t1;
> > > 2021-11-04 10:51:53.912 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES
> > > IN SCHEMA should be specified before the table/schema name(s) at
> > > character 26
> > > 2021-11-04 10:51:53.912 AEDT [738] STATEMENT:  ALTER PUBLICATION p1
> > > SET t1;
> > > ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified
> > > before the table/schema name(s) LINE 1: ALTER PUBLICATION p1 SET t1;
> > >  ^
> >
> > Thanks for the comment, I changed the error message to remove the FOR
> > keyword. The attached patch has the changes for the same.
> >
> 
> LGTM.

+1

Best regards,
Hou zj

Re: Added schema level support for publication.

[Peter Smith]

On Thu, Nov 4, 2021 at 3:24 PM vignesh C  wrote:
>
> On Thu, Nov 4, 2021 at 5:41 AM Peter Smith  wrote:
> >
> > FYI - I found a small problem with one of the new PublicationObjSpec
> > parser error messages that was introduced by the recent schema
> > publication commit [1].
> >
> > The error message text is assuming that the error originates from
> > CREATE PUBLICATION, but actually that same error can also come from
> > ALTER PUBLICATION.
> >
> > For example,
> >
> > e.g.1) Here the error came from CREATE PUBLICATION, so the message
> > text looks OK.
> >
> > test_pub=# CREATE PUBLICATION p1 FOR t1;
> > 2021-11-04 10:50:17.208 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
> > SCHEMA should be specified before the table/schema name(s) at
> > character 27
> > 2021-11-04 10:50:17.208 AEDT [738] STATEMENT:  CREATE PUBLICATION p1 FOR t1;
> > ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
> > the table/schema name(s)
> > LINE 1: CREATE PUBLICATION p1 FOR t1;
> >   ^
> > ~~
> >
> > e.g.2) Here the error came from ALTER PUBLICATION, so the message text
> > is not OK because the ALTER syntax [2] does not even have a FOR
> > keyword.
> >
> > test_pub=# ALTER PUBLICATION p1 SET t1;
> > 2021-11-04 10:51:53.912 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
> > SCHEMA should be specified before the table/schema name(s) at
> > character 26
> > 2021-11-04 10:51:53.912 AEDT [738] STATEMENT:  ALTER PUBLICATION p1 SET t1;
> > ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
> > the table/schema name(s)
> > LINE 1: ALTER PUBLICATION p1 SET t1;
> >  ^
>
> Thanks for the comment, I changed the error message to remove the FOR
> keyword. The attached patch has the changes for the same.
>

LGTM.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Added schema level support for publication.

[vignesh C]

On Thu, Nov 4, 2021 at 5:41 AM Peter Smith  wrote:
>
> FYI - I found a small problem with one of the new PublicationObjSpec
> parser error messages that was introduced by the recent schema
> publication commit [1].
>
> The error message text is assuming that the error originates from
> CREATE PUBLICATION, but actually that same error can also come from
> ALTER PUBLICATION.
>
> For example,
>
> e.g.1) Here the error came from CREATE PUBLICATION, so the message
> text looks OK.
>
> test_pub=# CREATE PUBLICATION p1 FOR t1;
> 2021-11-04 10:50:17.208 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
> SCHEMA should be specified before the table/schema name(s) at
> character 27
> 2021-11-04 10:50:17.208 AEDT [738] STATEMENT:  CREATE PUBLICATION p1 FOR t1;
> ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
> the table/schema name(s)
> LINE 1: CREATE PUBLICATION p1 FOR t1;
>   ^
> ~~
>
> e.g.2) Here the error came from ALTER PUBLICATION, so the message text
> is not OK because the ALTER syntax [2] does not even have a FOR
> keyword.
>
> test_pub=# ALTER PUBLICATION p1 SET t1;
> 2021-11-04 10:51:53.912 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
> SCHEMA should be specified before the table/schema name(s) at
> character 26
> 2021-11-04 10:51:53.912 AEDT [738] STATEMENT:  ALTER PUBLICATION p1 SET t1;
> ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
> the table/schema name(s)
> LINE 1: ALTER PUBLICATION p1 SET t1;
>  ^

Thanks for the comment, I changed the error message to remove the FOR
keyword. The attached patch has the changes for the same.

Regards,
Vignesh
From 8a36508d18144150d63812778a8c675a75d0f56d Mon Sep 17 00:00:00 2001
From: Vigneshwaran C 
Date: Wed, 3 Nov 2021 09:21:22 +0530
Subject: [PATCH v3] Renamed few enums and changed error message in publish
 schema feature.

Renamed PUBLICATIONOBJ_REL_IN_SCHEMA, PUBLICATIONOBJ_CURRSCHEMA,
DO_PUBLICATION_REL_IN_SCHEMA and PRIO_PUBLICATION_REL_IN_SCHEMA
to PUBLICATIONOBJ_TABLE_IN_SCHEMA, PUBLICATIONOBJ_TABLE_IN_CUR_SCHEMA,
DO_PUBLICATION_TABLE_IN_SCHEMA and PRIO_PUBLICATION_TABLE_IN_SCHEMA in publish
schema feature, this change is required to avoid confusion with other objects
like sequences which will be available in the future and also changed
the error message to keep it common for both create and alter publication.
---
 src/backend/commands/publicationcmds.c|  8 
 src/backend/parser/gram.y | 14 +++---
 src/bin/pg_dump/pg_dump.c |  6 +++---
 src/bin/pg_dump/pg_dump.h |  2 +-
 src/bin/pg_dump/pg_dump_sort.c|  6 +++---
 src/include/nodes/parsenodes.h|  5 +++--
 src/test/regress/expected/publication.out |  4 ++--
 7 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index d1fff13d2e..7d4a0e95f6 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -169,13 +169,13 @@ ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
 			case PUBLICATIONOBJ_TABLE:
 *rels = lappend(*rels, pubobj->pubtable);
 break;
-			case PUBLICATIONOBJ_REL_IN_SCHEMA:
+			case PUBLICATIONOBJ_TABLE_IN_SCHEMA:
 schemaid = get_namespace_oid(pubobj->name, false);
 
 /* Filter out duplicates if user specifies "sch1, sch1" */
 *schemas = list_append_unique_oid(*schemas, schemaid);
 break;
-			case PUBLICATIONOBJ_CURRSCHEMA:
+			case PUBLICATIONOBJ_TABLE_IN_CUR_SCHEMA:
 search_path = fetch_search_path(false);
 if (search_path == NIL) /* nothing valid in search_path? */
 	ereport(ERROR,
@@ -214,7 +214,7 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
 
 		if (list_member_oid(schemaidlist, relSchemaId))
 		{
-			if (checkobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA)
+			if (checkobjtype == PUBLICATIONOBJ_TABLE_IN_SCHEMA)
 ereport(ERROR,
 		errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 		errmsg("cannot add schema \"%s\" to publication",
@@ -613,7 +613,7 @@ AlterPublicationSchemas(AlterPublicationStmt *stmt,
 		rels = OpenReliIdList(reloids);
 
 		CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
-			  PUBLICATIONOBJ_REL_IN_SCHEMA);
+			  PUBLICATIONOBJ_TABLE_IN_SCHEMA);
 
 		CloseTableList(rels);
 		PublicationAddSchemas(pubform->oid, schemaidlist, false, stmt);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d0eb80e69c..a6d0cefa6b 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9662,14 +9662,14 @@ PublicationObjSpec:
 			| ALL TABLES IN_P SCHEMA ColId
 {
 	$$ = makeNode(PublicationObjSpec);
-	$$->pubobjtype = PUBLICATIONOBJ_REL_IN_SCHEMA;
+	$$->pubobjtype = PUBLICATIONOBJ_TABLE_IN_SCHEMA;
 	$$->name = $5;
 	$$->location = @5;
 }
 			| ALL TABLES IN_P SCHEMA CURRENT_SCHEMA
 {
 	$$ 

Re: Added schema level support for publication.

[Peter Smith]

On Thu, Nov 4, 2021 at 11:55 AM houzj.f...@fujitsu.com
 wrote:
>
> On Thurs, Nov 4, 2021 8:12 AM Peter Smith  wrote:
> > FYI - I found a small problem with one of the new PublicationObjSpec parser
> > error messages that was introduced by the recent schema publication commit
> > [1].
> >
> > The error message text is assuming that the error originates from CREATE
> > PUBLICATION, but actually that same error can also come from ALTER
> > PUBLICATION.
> > e.g.2) Here the error came from ALTER PUBLICATION, so the message text is
> > not OK because the ALTER syntax [2] does not even have a FOR keyword.
> >
> > test_pub=# ALTER PUBLICATION p1 SET t1;
> > 2021-11-04 10:51:53.912 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
> > SCHEMA should be specified before the table/schema name(s) at character 26
> > 2021-11-04 10:51:53.912 AEDT [738] STATEMENT:  ALTER PUBLICATION p1 SET
> > t1;
> > ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
> > the table/schema name(s) LINE 1: ALTER PUBLICATION p1 SET t1;
>
> I think it might be better to report " TABLE/ALL TABLES IN SCHEMA should be 
> specified before ...".
>

+1 - Yes, I also thought the fix should just be some simple/generic
change of the wording like your suggestion (rather than a different
message for both cases).

--
Kind Regards,
Peter Smith.
Fujitsu Australia

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Thurs, Nov 4, 2021 8:12 AM Peter Smith  wrote:
> FYI - I found a small problem with one of the new PublicationObjSpec parser
> error messages that was introduced by the recent schema publication commit
> [1].
> 
> The error message text is assuming that the error originates from CREATE
> PUBLICATION, but actually that same error can also come from ALTER
> PUBLICATION.
> e.g.2) Here the error came from ALTER PUBLICATION, so the message text is
> not OK because the ALTER syntax [2] does not even have a FOR keyword.
> 
> test_pub=# ALTER PUBLICATION p1 SET t1;
> 2021-11-04 10:51:53.912 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
> SCHEMA should be specified before the table/schema name(s) at character 26
> 2021-11-04 10:51:53.912 AEDT [738] STATEMENT:  ALTER PUBLICATION p1 SET
> t1;
> ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
> the table/schema name(s) LINE 1: ALTER PUBLICATION p1 SET t1;

I think it might be better to report " TABLE/ALL TABLES IN SCHEMA should be 
specified before ...".

Best regards,
Hou zj

Re: Added schema level support for publication.

[Peter Smith]

FYI - I found a small problem with one of the new PublicationObjSpec
parser error messages that was introduced by the recent schema
publication commit [1].

The error message text is assuming that the error originates from
CREATE PUBLICATION, but actually that same error can also come from
ALTER PUBLICATION.

For example,

e.g.1) Here the error came from CREATE PUBLICATION, so the message
text looks OK.

test_pub=# CREATE PUBLICATION p1 FOR t1;
2021-11-04 10:50:17.208 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
SCHEMA should be specified before the table/schema name(s) at
character 27
2021-11-04 10:50:17.208 AEDT [738] STATEMENT:  CREATE PUBLICATION p1 FOR t1;
ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
the table/schema name(s)
LINE 1: CREATE PUBLICATION p1 FOR t1;
  ^
~~

e.g.2) Here the error came from ALTER PUBLICATION, so the message text
is not OK because the ALTER syntax [2] does not even have a FOR
keyword.

test_pub=# ALTER PUBLICATION p1 SET t1;
2021-11-04 10:51:53.912 AEDT [738] ERROR:  FOR TABLE/FOR ALL TABLES IN
SCHEMA should be specified before the table/schema name(s) at
character 26
2021-11-04 10:51:53.912 AEDT [738] STATEMENT:  ALTER PUBLICATION p1 SET t1;
ERROR:  FOR TABLE/FOR ALL TABLES IN SCHEMA should be specified before
the table/schema name(s)
LINE 1: ALTER PUBLICATION p1 SET t1;
 ^
--
[1] 
https://github.com/postgres/postgres/commit/5a2832465fd8984d089e8c44c094e6900d987fcd
[2] https://www.postgresql.org/docs/devel/sql-alterpublication.html

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

On Wed, Nov 3, 2021 at 11:55 AM vignesh C  wrote:
>
> On Wed, Nov 3, 2021 at 11:37 AM Peter Smith  wrote:
> >
> > While you are changing these, maybe also change:
> >
> > Before: PUBLICATIONOBJ..._CURRSCHEMA
> > After: PUBLICATIONOBJ..._CUR_SCHEMA
> >
> > Which seems more readable to me.
>
> Thanks for the comment, the attached patch has the changes for the same.
>

LGTM. I'll push this tomorrow unless there are more comments or suggestions.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[vignesh C]

On Wed, Nov 3, 2021 at 11:07 AM houzj.f...@fujitsu.com
 wrote:
>
> On Wed, Nov 3, 2021 12:25 PM vignesh C  wrote:
> > On Wed, Nov 3, 2021 at 8:30 AM Amit Kapila 
> > wrote:
> > >
> > > On Tue, Nov 2, 2021 at 8:13 PM Tomas Vondra
> > >  wrote:
> > > >
> > > >
> > > > >
> > > > > Yeah, that is also true. So maybe at this, we can just rename the
> > > > > few types as suggested by you and we can look at it later if we
> > > > > anytime have more number of objects to add.
> > > > >
> > > >
> > > > +1
> > > >
> > >
> > > Apart from what you have pointed above, we are using
> > > "DO_PUBLICATION_REL_IN_SCHEMA" in pg_dump. I think we should replace
> > > that as well with "DO_PUBLICATION_TABLE_IN_SCHEMA".
> >
> > Thanks for the comments, the attached patch has the changes for the same.
> >
>
> Thanks for the patch.
> I have only one minor comment:
>
> +   PUBLICATIONOBJ_TABLE_IN_SCHEMA, /* Relations in schema type */
>
> I think the ' Relations' in the comments also need to be changed to 'tables'.
>
> The other part of the patch looks good to me.

Thanks for the comment, the patch at [1] has the changes for the same.
[1] - 
https://www.postgresql.org/message-id/CALDaNm3g4ZaJ8h%3D16_A%2BytbyPUdJPaAz94YzQLqkD%3DyPu%2BVPwA%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Wed, Nov 3, 2021 at 11:37 AM Peter Smith  wrote:
>
> While you are changing these, maybe also change:
>
> Before: PUBLICATIONOBJ..._CURRSCHEMA
> After: PUBLICATIONOBJ..._CUR_SCHEMA
>
> Which seems more readable to me.

Thanks for the comment, the attached patch has the changes for the same.

Regards,
Vignesh
From 41bc108cee08f1a7e488b6b801e581e212dcb0a3 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C 
Date: Wed, 3 Nov 2021 09:21:22 +0530
Subject: [PATCH v2] Renamed enums that was specified as REL to TABLE in
 publish schema feature.

Renamed PUBLICATIONOBJ_REL_IN_SCHEMA, PUBLICATIONOBJ_CURRSCHEMA,
DO_PUBLICATION_REL_IN_SCHEMA and PRIO_PUBLICATION_REL_IN_SCHEMA
to PUBLICATIONOBJ_TABLE_IN_SCHEMA, PUBLICATIONOBJ_TABLE_IN_CURRSCHEMA,
DO_PUBLICATION_TABLE_IN_SCHEMA and PRIO_PUBLICATION_TABLE_IN_SCHEMA in publish
schema feature, this change is required to avoid confusion with other objects
like sequences which will be available in the future.
---
 src/backend/commands/publicationcmds.c |  8 
 src/backend/parser/gram.y  | 12 ++--
 src/bin/pg_dump/pg_dump.c  |  6 +++---
 src/bin/pg_dump/pg_dump.h  |  2 +-
 src/bin/pg_dump/pg_dump_sort.c |  6 +++---
 src/include/nodes/parsenodes.h |  5 +++--
 6 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index d1fff13d2e..7d4a0e95f6 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -169,13 +169,13 @@ ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
 			case PUBLICATIONOBJ_TABLE:
 *rels = lappend(*rels, pubobj->pubtable);
 break;
-			case PUBLICATIONOBJ_REL_IN_SCHEMA:
+			case PUBLICATIONOBJ_TABLE_IN_SCHEMA:
 schemaid = get_namespace_oid(pubobj->name, false);
 
 /* Filter out duplicates if user specifies "sch1, sch1" */
 *schemas = list_append_unique_oid(*schemas, schemaid);
 break;
-			case PUBLICATIONOBJ_CURRSCHEMA:
+			case PUBLICATIONOBJ_TABLE_IN_CUR_SCHEMA:
 search_path = fetch_search_path(false);
 if (search_path == NIL) /* nothing valid in search_path? */
 	ereport(ERROR,
@@ -214,7 +214,7 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
 
 		if (list_member_oid(schemaidlist, relSchemaId))
 		{
-			if (checkobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA)
+			if (checkobjtype == PUBLICATIONOBJ_TABLE_IN_SCHEMA)
 ereport(ERROR,
 		errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 		errmsg("cannot add schema \"%s\" to publication",
@@ -613,7 +613,7 @@ AlterPublicationSchemas(AlterPublicationStmt *stmt,
 		rels = OpenReliIdList(reloids);
 
 		CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
-			  PUBLICATIONOBJ_REL_IN_SCHEMA);
+			  PUBLICATIONOBJ_TABLE_IN_SCHEMA);
 
 		CloseTableList(rels);
 		PublicationAddSchemas(pubform->oid, schemaidlist, false, stmt);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d0eb80e69c..beccdfb2fb 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9662,14 +9662,14 @@ PublicationObjSpec:
 			| ALL TABLES IN_P SCHEMA ColId
 {
 	$$ = makeNode(PublicationObjSpec);
-	$$->pubobjtype = PUBLICATIONOBJ_REL_IN_SCHEMA;
+	$$->pubobjtype = PUBLICATIONOBJ_TABLE_IN_SCHEMA;
 	$$->name = $5;
 	$$->location = @5;
 }
 			| ALL TABLES IN_P SCHEMA CURRENT_SCHEMA
 {
 	$$ = makeNode(PublicationObjSpec);
-	$$->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
+	$$->pubobjtype = PUBLICATIONOBJ_TABLE_IN_CUR_SCHEMA;
 	$$->location = @5;
 }
 			| ColId
@@ -17351,17 +17351,17 @@ preprocess_pubobj_list(List *pubobjspec_list, core_yyscan_t yyscanner)
 pubobj->name = NULL;
 			}
 		}
-		else if (pubobj->pubobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA ||
- pubobj->pubobjtype == PUBLICATIONOBJ_CURRSCHEMA)
+		else if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE_IN_SCHEMA ||
+ pubobj->pubobjtype == PUBLICATIONOBJ_TABLE_IN_CUR_SCHEMA)
 		{
 			/*
 			 * We can distinguish between the different type of schema
 			 * objects based on whether name and pubtable is set.
 			 */
 			if (pubobj->name)
-pubobj->pubobjtype = PUBLICATIONOBJ_REL_IN_SCHEMA;
+pubobj->pubobjtype = PUBLICATIONOBJ_TABLE_IN_SCHEMA;
 			else if (!pubobj->name && !pubobj->pubtable)
-pubobj->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
+pubobj->pubobjtype = PUBLICATIONOBJ_TABLE_IN_CUR_SCHEMA;
 			else
 ereport(ERROR,
 		errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index b9635a95b6..7e98371d25 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4194,7 +4194,7 @@ getPublicationNamespaces(Archive *fout)
 			continue;
 
 		/* OK, make a DumpableObject for this relationship */
-		pubsinfo[j].dobj.objType = DO_PUBLICATION_REL_IN_SCHEMA;
+		pubsinfo[j].dobj.objType = DO_PUBLICATION_TABLE_IN_SCHEMA;
 		

Re: Added schema level support for publication.

[Peter Smith]

While you are changing these, maybe also change:

Before: PUBLICATIONOBJ..._CURRSCHEMA
After: PUBLICATIONOBJ..._CUR_SCHEMA

Which seems more readable to me.

--
Kind Regards,
Peter Smith.
Fujitsu Australia

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Wed, Nov 3, 2021 12:25 PM vignesh C  wrote:
> On Wed, Nov 3, 2021 at 8:30 AM Amit Kapila 
> wrote:
> >
> > On Tue, Nov 2, 2021 at 8:13 PM Tomas Vondra
> >  wrote:
> > >
> > >
> > > >
> > > > Yeah, that is also true. So maybe at this, we can just rename the
> > > > few types as suggested by you and we can look at it later if we
> > > > anytime have more number of objects to add.
> > > >
> > >
> > > +1
> > >
> >
> > Apart from what you have pointed above, we are using
> > "DO_PUBLICATION_REL_IN_SCHEMA" in pg_dump. I think we should replace
> > that as well with "DO_PUBLICATION_TABLE_IN_SCHEMA".
> 
> Thanks for the comments, the attached patch has the changes for the same.
> 

Thanks for the patch.
I have only one minor comment:

+   PUBLICATIONOBJ_TABLE_IN_SCHEMA, /* Relations in schema type */

I think the ' Relations' in the comments also need to be changed to 'tables'.

The other part of the patch looks good to me.

Best regards,
Hou zj

Re: Added schema level support for publication.

[vignesh C]

On Wed, Nov 3, 2021 at 8:30 AM Amit Kapila  wrote:
>
> On Tue, Nov 2, 2021 at 8:13 PM Tomas Vondra
>  wrote:
> >
> >
> > >
> > > Yeah, that is also true. So maybe at this, we can just rename the few
> > > types as suggested by you and we can look at it later if we anytime
> > > have more number of objects to add.
> > >
> >
> > +1
> >
>
> Apart from what you have pointed above, we are using
> "DO_PUBLICATION_REL_IN_SCHEMA" in pg_dump. I think we should replace
> that as well with "DO_PUBLICATION_TABLE_IN_SCHEMA".

Thanks for the comments, the attached patch has the changes for the same.

Regards,
Vignesh
From 533de4b3024e12b033d82995a3b920b51f2a7d64 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C 
Date: Wed, 3 Nov 2021 09:21:22 +0530
Subject: [PATCH] Renamed enums that included REL to TABLE in publish schema
 feature.

Renamed PUBLICATIONOBJ_REL_IN_SCHEMA, PUBLICATIONOBJ_CURRSCHEMA,
DO_PUBLICATION_REL_IN_SCHEMA and PRIO_PUBLICATION_REL_IN_SCHEMA
to PUBLICATIONOBJ_TABLE_IN_SCHEMA, PUBLICATIONOBJ_TABLE_IN_CURRSCHEMA,
DO_PUBLICATION_TABLE_IN_SCHEMA and PRIO_PUBLICATION_TABLE_IN_SCHEMA in publish
schema feature, this change is required to avoid confusion with other objects
like sequences which will be available in the future.
---
 src/backend/commands/publicationcmds.c |  8 
 src/backend/parser/gram.y  | 12 ++--
 src/bin/pg_dump/pg_dump.c  |  6 +++---
 src/bin/pg_dump/pg_dump.h  |  2 +-
 src/bin/pg_dump/pg_dump_sort.c |  6 +++---
 src/include/nodes/parsenodes.h |  5 +++--
 6 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index d1fff13d2e..c8c45d9426 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -169,13 +169,13 @@ ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
 			case PUBLICATIONOBJ_TABLE:
 *rels = lappend(*rels, pubobj->pubtable);
 break;
-			case PUBLICATIONOBJ_REL_IN_SCHEMA:
+			case PUBLICATIONOBJ_TABLE_IN_SCHEMA:
 schemaid = get_namespace_oid(pubobj->name, false);
 
 /* Filter out duplicates if user specifies "sch1, sch1" */
 *schemas = list_append_unique_oid(*schemas, schemaid);
 break;
-			case PUBLICATIONOBJ_CURRSCHEMA:
+			case PUBLICATIONOBJ_TABLE_IN_CURRSCHEMA:
 search_path = fetch_search_path(false);
 if (search_path == NIL) /* nothing valid in search_path? */
 	ereport(ERROR,
@@ -214,7 +214,7 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
 
 		if (list_member_oid(schemaidlist, relSchemaId))
 		{
-			if (checkobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA)
+			if (checkobjtype == PUBLICATIONOBJ_TABLE_IN_SCHEMA)
 ereport(ERROR,
 		errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 		errmsg("cannot add schema \"%s\" to publication",
@@ -613,7 +613,7 @@ AlterPublicationSchemas(AlterPublicationStmt *stmt,
 		rels = OpenReliIdList(reloids);
 
 		CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
-			  PUBLICATIONOBJ_REL_IN_SCHEMA);
+			  PUBLICATIONOBJ_TABLE_IN_SCHEMA);
 
 		CloseTableList(rels);
 		PublicationAddSchemas(pubform->oid, schemaidlist, false, stmt);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d0eb80e69c..8683dc1a37 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9662,14 +9662,14 @@ PublicationObjSpec:
 			| ALL TABLES IN_P SCHEMA ColId
 {
 	$$ = makeNode(PublicationObjSpec);
-	$$->pubobjtype = PUBLICATIONOBJ_REL_IN_SCHEMA;
+	$$->pubobjtype = PUBLICATIONOBJ_TABLE_IN_SCHEMA;
 	$$->name = $5;
 	$$->location = @5;
 }
 			| ALL TABLES IN_P SCHEMA CURRENT_SCHEMA
 {
 	$$ = makeNode(PublicationObjSpec);
-	$$->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
+	$$->pubobjtype = PUBLICATIONOBJ_TABLE_IN_CURRSCHEMA;
 	$$->location = @5;
 }
 			| ColId
@@ -17351,17 +17351,17 @@ preprocess_pubobj_list(List *pubobjspec_list, core_yyscan_t yyscanner)
 pubobj->name = NULL;
 			}
 		}
-		else if (pubobj->pubobjtype == PUBLICATIONOBJ_REL_IN_SCHEMA ||
- pubobj->pubobjtype == PUBLICATIONOBJ_CURRSCHEMA)
+		else if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE_IN_SCHEMA ||
+ pubobj->pubobjtype == PUBLICATIONOBJ_TABLE_IN_CURRSCHEMA)
 		{
 			/*
 			 * We can distinguish between the different type of schema
 			 * objects based on whether name and pubtable is set.
 			 */
 			if (pubobj->name)
-pubobj->pubobjtype = PUBLICATIONOBJ_REL_IN_SCHEMA;
+pubobj->pubobjtype = PUBLICATIONOBJ_TABLE_IN_SCHEMA;
 			else if (!pubobj->name && !pubobj->pubtable)
-pubobj->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
+pubobj->pubobjtype = PUBLICATIONOBJ_TABLE_IN_CURRSCHEMA;
 			else
 ereport(ERROR,
 		errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index b9635a95b6..7e98371d25 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ 

Re: Added schema level support for publication.

[Amit Kapila]

On Tue, Nov 2, 2021 at 8:13 PM Tomas Vondra
 wrote:
>
>
> >
> > Yeah, that is also true. So maybe at this, we can just rename the few
> > types as suggested by you and we can look at it later if we anytime
> > have more number of objects to add.
> >
>
> +1
>

Apart from what you have pointed above, we are using
"DO_PUBLICATION_REL_IN_SCHEMA" in pg_dump. I think we should replace
that as well with "DO_PUBLICATION_TABLE_IN_SCHEMA".

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Tomas Vondra]

On 11/2/21 11:37 AM, Amit Kapila wrote:
> On Mon, Nov 1, 2021 at 5:52 PM Tomas Vondra
>  wrote:
>>
>> On 11/1/21 11:18, Amit Kapila wrote:
>>> On Mon, Nov 1, 2021 at 2:48 AM Tomas Vondra
>>>  wrote:
 I wonder if it'd be better to just separate the schema and object type
 specification, instead of mashing it into a single constant.

>>>
>>> Do you mean to say the syntax on the lines of Create Publication For
>>> Table t1, t2 Schema s1, s2;? If so, then originally the patch had the
>>> syntax on those lines but Tom pointed out that the meaning of such a
>>> syntax can change over a period of time and that can break apps [1]. I
>>> think the current syntax gives a lot of flexibility to users and we
>>> have some precedent for it as well.
>>>
>>
>> No, I'm not talking about the syntax at all - I'm talking about how we
>> represent it. PUBLICATIONOBJ_TABLE_CURRSCHEMA mixes the object type and
>> schema in the same constant, so I am wondering if we should just split
>> that into two pieces - one determining the schema, one determining the
>> object type. So PublicationObjSpec would have two fields instead of just
>> pubobjtype.
>>
>> The advantage would be we wouldn't need a whole lot of new constants for
>> each object type - adding sequences pretty much means adding
>>
>>  PUBLICATIONOBJ_SEQUENCE
>>  PUBLICATIONOBJ_SEQUENCE_IN_SCHEMA
>>  PUBLICATIONOBJ_SEQUENCE_CURRSCHEMA
>>
>> and after splitting we'd need just the first one.
>>
> 
> I see your point but OTOH, I think it will lead to additional checks
> in post-processing functions like ObjectsInPublicationToOids() as we
> have to always check both object type and schema to make decisions.
> 

True.

>> But maybe it's not
>> that bad, though. We don't expect all that many object types in
>> publications, I guess.
>>
> 
> Yeah, that is also true. So maybe at this, we can just rename the few
> types as suggested by you and we can look at it later if we anytime
> have more number of objects to add.
> 

+1

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Added schema level support for publication.

[Amit Kapila]

On Mon, Nov 1, 2021 at 5:52 PM Tomas Vondra
 wrote:
>
> On 11/1/21 11:18, Amit Kapila wrote:
> > On Mon, Nov 1, 2021 at 2:48 AM Tomas Vondra
> >  wrote:
> >> I wonder if it'd be better to just separate the schema and object type
> >> specification, instead of mashing it into a single constant.
> >>
> >
> > Do you mean to say the syntax on the lines of Create Publication For
> > Table t1, t2 Schema s1, s2;? If so, then originally the patch had the
> > syntax on those lines but Tom pointed out that the meaning of such a
> > syntax can change over a period of time and that can break apps [1]. I
> > think the current syntax gives a lot of flexibility to users and we
> > have some precedent for it as well.
> >
>
> No, I'm not talking about the syntax at all - I'm talking about how we
> represent it. PUBLICATIONOBJ_TABLE_CURRSCHEMA mixes the object type and
> schema in the same constant, so I am wondering if we should just split
> that into two pieces - one determining the schema, one determining the
> object type. So PublicationObjSpec would have two fields instead of just
> pubobjtype.
>
> The advantage would be we wouldn't need a whole lot of new constants for
> each object type - adding sequences pretty much means adding
>
>  PUBLICATIONOBJ_SEQUENCE
>  PUBLICATIONOBJ_SEQUENCE_IN_SCHEMA
>  PUBLICATIONOBJ_SEQUENCE_CURRSCHEMA
>
> and after splitting we'd need just the first one.
>

I see your point but OTOH, I think it will lead to additional checks
in post-processing functions like ObjectsInPublicationToOids() as we
have to always check both object type and schema to make decisions.

> But maybe it's not
> that bad, though. We don't expect all that many object types in
> publications, I guess.
>

Yeah, that is also true. So maybe at this, we can just rename the few
types as suggested by you and we can look at it later if we anytime
have more number of objects to add.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

On Tue, Nov 2, 2021 at 6:43 AM Masahiko Sawada  wrote:
>
> > I am just not sure if it is worth adding
> > such a view or we leave it to users to find that information via
> > querying individual views or system tables for objects.
>
> I've not looked at the patch for logical replication of sequences but
> the view becomes more useful once we support the new type of
> replication object? If so, we can consider this view again after the
> patch gets committed.
>

+1.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Masahiko Sawada]

On Mon, Nov 1, 2021 at 7:28 PM Amit Kapila  wrote:
>
> On Mon, Nov 1, 2021 at 2:38 PM Greg Nancarrow  wrote:
> >
> > On Mon, Nov 1, 2021 at 5:07 PM Masahiko Sawada  
> > wrote:
> > >
> > > I haven't followed the discussion on pg_publication_objects view but
> > > what is the primary use case of this view? If it's to list all tables
> > > published in a publication (e.g, "select * from pg_publication_objects
> > > where pubname = 'pub1'), pg_publication_objects view lacks the
> > > information of FOR ALL TABLES publications. And probably we can use
> > > pg_publication_tables instead. On the other hand, if it's to list all
> > > tables published in FOR ALL TABLES IN SCHEMA publications (e.g.,
> > > "select * from pg_publication_object where objtype = 'schema'), the
> > > view doesn't show tables published in such publications.
> > >
>
> Both the problems mentioned can be fixed if we follow the change
> suggested by me in one of the emails above [1].
>
> >
> > I think that Amit originally suggested to have a view that provides
> > information about the objects in each publication (like table, tables
> > in schema, sequence ...).
> >
>
> Right and I think as you also mentioned in your previous email it can
> save the effort of users if they want to know all the objects
> published via a publication.

Thank you for the explanation. Given we already have
pg_publication_tables view, if pg_publication_objects view also shows
all tables published in FOR ALL TABLES publications or FOR ALL TABLES
IN SCHEMA publications, there is essentially not much difference
between pg_publication_tables and pg_publication_objects except for
objtype column. Right? If so it'd be better to have one row for each
FOR ALL TABLES publication and FOR ALL TABLES IN SCHEMA publication
with objtype = 'database' or 'schema' etc, instead of individual
tables.

> I am just not sure if it is worth adding
> such a view or we leave it to users to find that information via
> querying individual views or system tables for objects.

I've not looked at the patch for logical replication of sequences but
the view becomes more useful once we support the new type of
replication object? If so, we can consider this view again after the
patch gets committed.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Added schema level support for publication.

[Tomas Vondra]

On 11/1/21 11:18, Amit Kapila wrote:

On Mon, Nov 1, 2021 at 2:48 AM Tomas Vondra
 wrote:


On 10/28/21 04:41, Amit Kapila wrote:

On Mon, Oct 25, 2021 at 3:09 PM Amit Kapila  wrote:


On Mon, Oct 25, 2021 at 1:11 PM vignesh C  wrote:


I have fixed this in the v47 version attached.



Thanks, the first patch in the series "Allow publishing the tables of
schema." looks good to me. Unless there are more
comments/bugs/objections, I am planning to commit it in a day or so.



Yesterday, I have pushed the first patch. Feel free to submit the
remaining patches.



I haven't been following this thread recently, but while rebasing the
sequence decoding patch I noticed this adds

  PUBLICATIONOBJ_TABLE,/* Table type */
  PUBLICATIONOBJ_REL_IN_SCHEMA,/* Relations in schema type */

Shouldn't it be PUBLICATIONOBJ_TABLE_IN_SCHEMA, or why does it use rel
instead of table?



Yeah, it should be PUBLICATIONOBJ_TABLE_IN_SCHEMA considering we have
to add other objects like sequence.


I'm asking because the sequence decoding patch mimics ALTER PUBLICATION
options for sequences, including ALL SEQUENCES IN SCHEMA etc. and this
seems ambiguous. The same issue applies to PUBLICATIONOBJ_CURRSCHEMA,
which does not specify the object type.



I think we should name it PUBLICATIONOBJ_TABLE_CURRSCHEMA. Does that make sense?


I wonder if it'd be better to just separate the schema and object type
specification, instead of mashing it into a single constant.



Do you mean to say the syntax on the lines of Create Publication For
Table t1, t2 Schema s1, s2;? If so, then originally the patch had the
syntax on those lines but Tom pointed out that the meaning of such a
syntax can change over a period of time and that can break apps [1]. I
think the current syntax gives a lot of flexibility to users and we
have some precedent for it as well.



No, I'm not talking about the syntax at all - I'm talking about how we 
represent it. PUBLICATIONOBJ_TABLE_CURRSCHEMA mixes the object type and 
schema in the same constant, so I am wondering if we should just split 
that into two pieces - one determining the schema, one determining the 
object type. So PublicationObjSpec would have two fields instead of just 
pubobjtype.


The advantage would be we wouldn't need a whole lot of new constants for 
each object type - adding sequences pretty much means adding


PUBLICATIONOBJ_SEQUENCE
PUBLICATIONOBJ_SEQUENCE_IN_SCHEMA
PUBLICATIONOBJ_SEQUENCE_CURRSCHEMA

and after splitting we'd need just the first one. But maybe it's not 
that bad, though. We don't expect all that many object types in 
publications, I guess.



regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Added schema level support for publication.

[Amit Kapila]

On Mon, Nov 1, 2021 at 2:38 PM Greg Nancarrow  wrote:
>
> On Mon, Nov 1, 2021 at 5:07 PM Masahiko Sawada  wrote:
> >
> > I haven't followed the discussion on pg_publication_objects view but
> > what is the primary use case of this view? If it's to list all tables
> > published in a publication (e.g, "select * from pg_publication_objects
> > where pubname = 'pub1'), pg_publication_objects view lacks the
> > information of FOR ALL TABLES publications. And probably we can use
> > pg_publication_tables instead. On the other hand, if it's to list all
> > tables published in FOR ALL TABLES IN SCHEMA publications (e.g.,
> > "select * from pg_publication_object where objtype = 'schema'), the
> > view doesn't show tables published in such publications.
> >

Both the problems mentioned can be fixed if we follow the change
suggested by me in one of the emails above [1].

>
> I think that Amit originally suggested to have a view that provides
> information about the objects in each publication (like table, tables
> in schema, sequence ...).
>

Right and I think as you also mentioned in your previous email it can
save the effort of users if they want to know all the objects
published via a publication. I am just not sure if it is worth adding
such a view or we leave it to users to find that information via
querying individual views or system tables for objects.

[1] - 
https://www.postgresql.org/message-id/CAA4eK1%2BL2-6JQ174sVfE3_K%3DmjTKJ2A8-z%2B_pExDHhqdBJvb5Q%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

On Mon, Nov 1, 2021 at 2:48 AM Tomas Vondra
 wrote:
>
> On 10/28/21 04:41, Amit Kapila wrote:
> > On Mon, Oct 25, 2021 at 3:09 PM Amit Kapila  wrote:
> >>
> >> On Mon, Oct 25, 2021 at 1:11 PM vignesh C  wrote:
> >>>
> >>> I have fixed this in the v47 version attached.
> >>>
> >>
> >> Thanks, the first patch in the series "Allow publishing the tables of
> >> schema." looks good to me. Unless there are more
> >> comments/bugs/objections, I am planning to commit it in a day or so.
> >>
> >
> > Yesterday, I have pushed the first patch. Feel free to submit the
> > remaining patches.
> >
>
> I haven't been following this thread recently, but while rebasing the
> sequence decoding patch I noticed this adds
>
>  PUBLICATIONOBJ_TABLE,/* Table type */
>  PUBLICATIONOBJ_REL_IN_SCHEMA,/* Relations in schema type */
>
> Shouldn't it be PUBLICATIONOBJ_TABLE_IN_SCHEMA, or why does it use rel
> instead of table?
>

Yeah, it should be PUBLICATIONOBJ_TABLE_IN_SCHEMA considering we have
to add other objects like sequence.

> I'm asking because the sequence decoding patch mimics ALTER PUBLICATION
> options for sequences, including ALL SEQUENCES IN SCHEMA etc. and this
> seems ambiguous. The same issue applies to PUBLICATIONOBJ_CURRSCHEMA,
> which does not specify the object type.
>

I think we should name it PUBLICATIONOBJ_TABLE_CURRSCHEMA. Does that make sense?

> I wonder if it'd be better to just separate the schema and object type
> specification, instead of mashing it into a single constant.
>

Do you mean to say the syntax on the lines of Create Publication For
Table t1, t2 Schema s1, s2;? If so, then originally the patch had the
syntax on those lines but Tom pointed out that the meaning of such a
syntax can change over a period of time and that can break apps [1]. I
think the current syntax gives a lot of flexibility to users and we
have some precedent for it as well.

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

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Greg Nancarrow]

On Mon, Nov 1, 2021 at 5:07 PM Masahiko Sawada  wrote:
>
> I haven't followed the discussion on pg_publication_objects view but
> what is the primary use case of this view? If it's to list all tables
> published in a publication (e.g, "select * from pg_publication_objects
> where pubname = 'pub1'), pg_publication_objects view lacks the
> information of FOR ALL TABLES publications. And probably we can use
> pg_publication_tables instead. On the other hand, if it's to list all
> tables published in FOR ALL TABLES IN SCHEMA publications (e.g.,
> "select * from pg_publication_object where objtype = 'schema'), the
> view doesn't show tables published in such publications.
>

I think that Amit originally suggested to have a view that provides
information about the objects in each publication (like table, tables
in schema, sequence ...).
So it currently is at the granularity level of the objects that are
actually added to the publication (TABLE t, ALL TABLES IN SCHEMA s)
I agree that the view is currently missing ALL TABLES publication
information, but I think it could be easily added.
Also, currently for the "objtype" column, the type "schema" does not
seem specific enough; maybe that should be instead named
"all-tables-in-schema" or similar.


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Masahiko Sawada]

On Fri, Oct 29, 2021 at 1:35 PM Amit Kapila  wrote:
>
> On Thu, Oct 28, 2021 at 9:55 AM vignesh C  wrote:
> >
> > Thanks for committing the patch, please find the remaining patches attached.
> > Thanks Hou Zhijie and Greg Nancarrow for sharing a few comments
> > offline, I have fixed those in the attached patch.
> >
>
> Pushed the first test case patch. About
> v48-0002-Add-new-pg_publication_objects-view-to-display-T, I think it
> doesn't display anything for "for all tables" publication. Instead of
> selecting from pg_publication_rel, you can use the existing view
> pg_publication_tables to solve that problem.
>
> Having said that, I am not completely sure about the value of this new
> view pg_publication_objects which displays all objects of
> publications. I see that users might want to see all the objects that
> the publication publishes and when we include other objects like
> sequences it might be more helpful.
>
> Sawada-San, others, what do you think? Is it really useful to have such a 
> view?

I haven't followed the discussion on pg_publication_objects view but
what is the primary use case of this view? If it's to list all tables
published in a publication (e.g, "select * from pg_publication_objects
where pubname = 'pub1'), pg_publication_objects view lacks the
information of FOR ALL TABLES publications. And probably we can use
pg_publication_tables instead. On the other hand, if it's to list all
tables published in FOR ALL TABLES IN SCHEMA publications (e.g.,
"select * from pg_publication_object where objtype = 'schema'), the
view doesn't show tables published in such publications.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Added schema level support for publication.

[Greg Nancarrow]

On Fri, Oct 29, 2021 at 3:35 PM Amit Kapila  wrote:
>
> Sawada-San, others, what do you think? Is it really useful to have such a 
> view?
>
> One point to think is if we introduce such a view then how it should
> behave for schema objects? Do we need to display only schemas or
> additionally all the tables in the schema as well? If you follow the
> above suggestion of mine then I think it will display both schemas
> published and tables in that schema that will be considered for
> publishing.
>

I find the proposed view useful for processing the publication
structure and members in SQL, without having to piece the information
together from the other pg_publication_* tables.
Personally I don't think it is necessary to additionally display all
tables in the schema (that information can be retrieved by pg_tables
or information_schema.tables, if needed).


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Tomas Vondra]

Hi,

On 10/28/21 04:41, Amit Kapila wrote:

On Mon, Oct 25, 2021 at 3:09 PM Amit Kapila  wrote:


On Mon, Oct 25, 2021 at 1:11 PM vignesh C  wrote:


I have fixed this in the v47 version attached.



Thanks, the first patch in the series "Allow publishing the tables of
schema." looks good to me. Unless there are more
comments/bugs/objections, I am planning to commit it in a day or so.



Yesterday, I have pushed the first patch. Feel free to submit the
remaining patches.



I haven't been following this thread recently, but while rebasing the 
sequence decoding patch I noticed this adds


PUBLICATIONOBJ_TABLE,/* Table type */
PUBLICATIONOBJ_REL_IN_SCHEMA,/* Relations in schema type */

Shouldn't it be PUBLICATIONOBJ_TABLE_IN_SCHEMA, or why does it use rel 
instead of table?


I'm asking because the sequence decoding patch mimics ALTER PUBLICATION 
options for sequences, including ALL SEQUENCES IN SCHEMA etc. and this 
seems ambiguous. The same issue applies to PUBLICATIONOBJ_CURRSCHEMA, 
which does not specify the object type.


I wonder if it'd be better to just separate the schema and object type 
specification, instead of mashing it into a single constant. Otherwise 
we'll end up with (M x N) combinations, which seems silly.



regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

RE: Added schema level support for publication.

[tanghy.f...@fujitsu.com]

On Friday, October 29, 2021 12:35 PM, Amit Kapila  
wrote:
> 
> On Thu, Oct 28, 2021 at 9:55 AM vignesh C  wrote:
> >
> > Thanks for committing the patch, please find the remaining patches attached.
> > Thanks Hou Zhijie and Greg Nancarrow for sharing a few comments
> > offline, I have fixed those in the attached patch.
> >
> 
> Pushed the first test case patch. About
> v48-0002-Add-new-pg_publication_objects-view-to-display-T, I think it
> doesn't display anything for "for all tables" publication. Instead of
> selecting from pg_publication_rel, you can use the existing view
> pg_publication_tables to solve that problem.
> 
> Having said that, I am not completely sure about the value of this new
> view pg_publication_objects which displays all objects of
> publications. I see that users might want to see all the objects that
> the publication publishes and when we include other objects like
> sequences it might be more helpful.
> 
> Sawada-San, others, what do you think? Is it really useful to have such a 
> view?
> 
> One point to think is if we introduce such a view then how it should
> behave for schema objects? Do we need to display only schemas or
> additionally all the tables in the schema as well? If you follow the
> above suggestion of mine then I think it will display both schemas
> published and tables in that schema that will be considered for
> publishing.
> 

Personally, if I want to see ALL the published objects in a publication, I 
would use
'\dRp+' command. I think there might not be too much value to have this view.

Regards
Tang

Re: Added schema level support for publication.

[Amit Kapila]

On Thu, Oct 28, 2021 at 9:55 AM vignesh C  wrote:
>
> Thanks for committing the patch, please find the remaining patches attached.
> Thanks Hou Zhijie and Greg Nancarrow for sharing a few comments
> offline, I have fixed those in the attached patch.
>

Pushed the first test case patch. About
v48-0002-Add-new-pg_publication_objects-view-to-display-T, I think it
doesn't display anything for "for all tables" publication. Instead of
selecting from pg_publication_rel, you can use the existing view
pg_publication_tables to solve that problem.

Having said that, I am not completely sure about the value of this new
view pg_publication_objects which displays all objects of
publications. I see that users might want to see all the objects that
the publication publishes and when we include other objects like
sequences it might be more helpful.

Sawada-San, others, what do you think? Is it really useful to have such a view?

One point to think is if we introduce such a view then how it should
behave for schema objects? Do we need to display only schemas or
additionally all the tables in the schema as well? If you follow the
above suggestion of mine then I think it will display both schemas
published and tables in that schema that will be considered for
publishing.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Greg Nancarrow]

On Thu, Oct 28, 2021 at 3:25 PM vignesh C  wrote:
>
> Thanks for committing the patch, please find the remaining patches attached.

A few comments on the v48-0002 patch:

(1) The quoting of TABLE/SCHEMA looks a bit odd in the patch comment

(2) src/backend/catalog/system_views.sq
ON should be capitalized in the following line:
+JOIN pg_catalog.pg_namespace N on N.oid = S.pnnspid

(3) Some basic tests should be added for this in the publication tests.


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[vignesh C]

On Thu, Oct 28, 2021 at 8:12 AM Amit Kapila  wrote:
>
> On Mon, Oct 25, 2021 at 3:09 PM Amit Kapila  wrote:
> >
> > On Mon, Oct 25, 2021 at 1:11 PM vignesh C  wrote:
> > >
> > > I have fixed this in the v47 version attached.
> > >
> >
> > Thanks, the first patch in the series "Allow publishing the tables of
> > schema." looks good to me. Unless there are more
> > comments/bugs/objections, I am planning to commit it in a day or so.
> >
>
> Yesterday, I have pushed the first patch. Feel free to submit the
> remaining patches.

Thanks for committing the patch, please find the remaining patches attached.
Thanks Hou Zhijie and Greg Nancarrow for sharing a few comments
offline, I have fixed those in the attached patch.

Regards,
Vignesh
From fa0fe25d1143575bdf31489bc8d1c88aa1ea362b Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Thu, 21 Oct 2021 14:25:24 +0530
Subject: [PATCH v48 1/2] Add tap tests for the schema publication feature of
 logical replication

Add tap tests for the schema publication feature of logical replication

Author: Vignesh C, Tang Haiying
Reviewed-by: Amit Kapila, Hou Zhijie, Greg Nancarrow, Masahiko Sawada, Peter Eisentraut, Tom Lane, Peter Smith, Ajin Cherian, Rahila Syed, Bharath Rupireddy, Mark Dilger
Tested-by: Tang Haiying
Discussion: https://www.postgresql.org/message-id/CALDaNm0OANxuJ6RXqwZsM1MSY4s19nuH3734j4a72etDwvBETQ%40mail.gmail.com
---
 .../t/025_rep_changes_for_schema.pl   | 205 ++
 1 file changed, 205 insertions(+)
 create mode 100644 src/test/subscription/t/025_rep_changes_for_schema.pl

diff --git a/src/test/subscription/t/025_rep_changes_for_schema.pl b/src/test/subscription/t/025_rep_changes_for_schema.pl
new file mode 100644
index 00..8f14da06a7
--- /dev/null
+++ b/src/test/subscription/t/025_rep_changes_for_schema.pl
@@ -0,0 +1,205 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# Logical replication tests for schema publications
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More tests => 13;
+
+# Initialize publisher node
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# Test replication with publications created using FOR ALL TABLES IN SCHEMA
+# option.
+# Create schemas and tables on publisher
+$node_publisher->safe_psql('postgres', "CREATE SCHEMA sch1");
+$node_publisher->safe_psql('postgres',
+	"CREATE TABLE sch1.tab1 AS SELECT generate_series(1,10) AS a");
+$node_publisher->safe_psql('postgres',
+	"CREATE TABLE sch1.tab2 AS SELECT generate_series(1,10) AS a");
+$node_publisher->safe_psql('postgres',
+"CREATE TABLE sch1.tab1_parent (a int PRIMARY KEY, b text) PARTITION BY LIST (a)");
+$node_publisher->safe_psql('postgres',
+"CREATE TABLE public.tab1_child1 PARTITION OF sch1.tab1_parent FOR VALUES IN (1, 2, 3)");
+$node_publisher->safe_psql('postgres',
+"CREATE TABLE public.tab1_child2 PARTITION OF sch1.tab1_parent FOR VALUES IN (4, 5, 6)");
+
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO sch1.tab1_parent values (1),(4)");
+
+# Create schemas and tables on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE SCHEMA sch1");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE sch1.tab1 (a int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE sch1.tab2 (a int)");
+$node_subscriber->safe_psql('postgres',
+"CREATE TABLE sch1.tab1_parent (a int PRIMARY KEY, b text) PARTITION BY LIST (a)");
+$node_subscriber->safe_psql('postgres',
+"CREATE TABLE public.tab1_child1 PARTITION OF sch1.tab1_parent FOR VALUES IN (1, 2, 3)");
+$node_subscriber->safe_psql('postgres',
+"CREATE TABLE public.tab1_child2 PARTITION OF sch1.tab1_parent FOR VALUES IN (4, 5, 6)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_schema FOR ALL TABLES IN SCHEMA sch1");
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub_schema CONNECTION '$publisher_connstr' PUBLICATION tap_pub_schema"
+);
+
+$node_publisher->wait_for_catchup('tap_sub_schema');
+
+# Also wait for initial table sync to finish
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+$node_subscriber->poll_query_until('postgres', $synced_query)
+  or die "Timed out while waiting for subscriber to synchronize data";
+
+# Check the schema table data is synced up
+my $result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM sch1.tab1");
+is($result, qq(10|1|10), 'check rows on subscriber catchup');
+
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT 

Re: Added schema level support for publication.

[Amit Kapila]

On Mon, Oct 25, 2021 at 3:09 PM Amit Kapila  wrote:
>
> On Mon, Oct 25, 2021 at 1:11 PM vignesh C  wrote:
> >
> > I have fixed this in the v47 version attached.
> >
>
> Thanks, the first patch in the series "Allow publishing the tables of
> schema." looks good to me. Unless there are more
> comments/bugs/objections, I am planning to commit it in a day or so.
>

Yesterday, I have pushed the first patch. Feel free to submit the
remaining patches.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

On Mon, Oct 25, 2021 at 1:11 PM vignesh C  wrote:
>
> On Mon, Oct 25, 2021 at 10:52 AM Amit Kapila  wrote:
> >
> > On Fri, Oct 22, 2021 at 8:56 PM vignesh C  wrote:
> > >
> >
> > I am getting a compilation error in the latest patch on HEAD. I think
> > was relying on some variable removed by a recent commit
> > 92316a4582a5714d4e494aaf90360860e7fec37a. While looking at that
> > compilation error, I observed that we don't need the second and third
> > parameters in pg_dump.c/getPublicationNamespaces() as those are not
> > getting used.
>
> I have fixed this in the v47 version attached.
>

Thanks, the first patch in the series "Allow publishing the tables of
schema." looks good to me. Unless there are more
comments/bugs/objections, I am planning to commit it in a day or so.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

On Fri, Oct 22, 2021 at 8:56 PM vignesh C  wrote:
>

I am getting a compilation error in the latest patch on HEAD. I think
was relying on some variable removed by a recent commit
92316a4582a5714d4e494aaf90360860e7fec37a. While looking at that
compilation error, I observed that we don't need the second and third
parameters in pg_dump.c/getPublicationNamespaces() as those are not
getting used.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

On Fri, Oct 22, 2021 at 11:59 AM Greg Nancarrow  wrote:
>
> On Fri, Oct 22, 2021 at 12:41 PM Greg Nancarrow  wrote:
> >
> > I was also previously concerned about what the behavior should be when
> > only including just the partitions of a partitioned table in a
> > publication using ALL TABLES IN SCHEMA and having
> > publish_via_partition_root=true. It seems to implicitly include the
> > partitioned table in the publication. But I did some testing and found
> > that this is the current behavior when only the partitions are
> > individually included in a publication using TABLE, so it seems to be
> > OK.
> >
>
> Thinking some more about this, it still may still be confusing to the
> user if not explicitly stated in the ALL TABLES IN SCHEMA case.
> How about adding some additional explanation to the end of the
> following paragraph:
>
> + 
> +  When a partitioned table is published via schema level publication, all
> +  of its existing and future partitions irrespective of it being from the
> +  publication schema or not are implicitly considered to be part of the
> +  publication. So, even operations that are performed directly on a
> +  partition are also published via publications that its ancestors are
> +  part of.
> + 
>
> Something like:
>
> Similarly, if a partition is published via schema level publication
> and publish_via_partition_root=true, the parent partitioned table is
> implicitly considered to be part of the publication, irrespective of
> it being from the publication schema or not.
>

I don't think we need to explicitly mention this in docs as this is
quite similar to what is happening for the "For Table" case as well
and it seems to be clarified by "publish_via_partition_root"
definition in Create Publication docs [1].

[1] - https://www.postgresql.org/docs/devel/sql-createpublication.html

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[vignesh C]

On Fri, Oct 22, 2021 at 1:03 PM Masahiko Sawada  wrote:
>
> On Fri, Oct 22, 2021 at 2:25 PM Amit Kapila  wrote:
> >
> > On Thu, Oct 21, 2021 at 6:47 PM vignesh C  wrote:
> > >
> > >
> > > Thanks for the comments, the attached v45 patch has the fix for the same.
> > >
> >
> > The first patch is mostly looking good to me apart from the below
> > minor comments:
>
> Let me share other minor comments on v45-0001 patch:
>
> >
> > 1.
> > +  
> > +   The catalog pg_publication_namespace contains 
> > the
> > +   mapping between schemas and publications in the database.  This is a
> > +   many-to-many mapping.
> >
> > There are extra spaces after mapping at the end which are not required.

Modified

> +   ADD and DROP  clauses will add and
> +   remove one or more tables/schemas from the publication.  Note that adding
> +   tables/schemas to a publication that is already subscribed to will 
> require a
>
> There is also an extra space after "adding".

Modified

> -[ FOR TABLE [ ONLY ]  class="parameter">table_name [ * ] [, ...]
> -  | FOR ALL TABLES ]
> +[ FOR ALL TABLES
> +  | FOR  class="parameter">publication_object [, ... ] ]
>
> Similarly, after "TABLES".

Modified

> +
> + 
> +  Specifying a table that is part of a schema specified by
> +  FOR ALL TABLES IN SCHEMA is not supported.
> + 
>
> And, after "by".

Modified

> ---
>
> +static void
> +AlterPublicationSchemas(AlterPublicationStmt *stmt,
> +HeapTuple tup, List
> *schemaidlist)
> +{
> (snip)
> +PublicationAddSchemas(pubform->oid, schemaidlist, true, 
> stmt);
> +}
> +
> +return;
> +}
>
> The "return" at the end of the function is not necessary.

Modified

> ---
> +if (pubobj->name)
> +pubobj->pubobjtype =
> PUBLICATIONOBJ_REL_IN_SCHEMA;
> +else if (!pubobj->name && !pubobj->pubtable)
> +pubobj->pubobjtype = 
> PUBLICATIONOBJ_CURRSCHEMA;
> +else if (!pubobj->name)
> +ereport(ERROR,
> +
> errcode(ERRCODE_SYNTAX_ERROR),
> +errmsg("invalid
> schema name at or near"),
> +
> parser_errposition(pubobj->location));
>
> I think it's better to change the last "else if" to just "else".

Modified

> ---
> +
> +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, schemarelids);
> +}
> +else
> +tables = relids;
> +
> +}
>
> There is an extra newline after "table = relids;".

Removed it

I have made this change in the v46 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm3kBrMO5EyEgK_TyOrBuw%2BRvdJ2mJfpWb5e8FbuKg2cQw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[vignesh C]

On Fri, Oct 22, 2021 at 11:59 AM Greg Nancarrow  wrote:
>
> On Fri, Oct 22, 2021 at 12:41 PM Greg Nancarrow  wrote:
> >
> > I was also previously concerned about what the behavior should be when
> > only including just the partitions of a partitioned table in a
> > publication using ALL TABLES IN SCHEMA and having
> > publish_via_partition_root=true. It seems to implicitly include the
> > partitioned table in the publication. But I did some testing and found
> > that this is the current behavior when only the partitions are
> > individually included in a publication using TABLE, so it seems to be
> > OK.
> >
>
> Thinking some more about this, it still may still be confusing to the
> user if not explicitly stated in the ALL TABLES IN SCHEMA case.
> How about adding some additional explanation to the end of the
> following paragraph:
>
> + 
> +  When a partitioned table is published via schema level publication, all
> +  of its existing and future partitions irrespective of it being from the
> +  publication schema or not are implicitly considered to be part of the
> +  publication. So, even operations that are performed directly on a
> +  partition are also published via publications that its ancestors are
> +  part of.
> + 
>
> Something like:
>
> Similarly, if a partition is published via schema level publication
> and publish_via_partition_root=true, the parent partitioned table is
> implicitly considered to be part of the publication, irrespective of
> it being from the publication schema or not.

I have made this change in the v46 patch attached at [1].
[1] - 
https://www.postgresql.org/message-id/CALDaNm3kBrMO5EyEgK_TyOrBuw%2BRvdJ2mJfpWb5e8FbuKg2cQw%40mail.gmail.com

Regards,
Vignesh

Re: Added schema level support for publication.

[Masahiko Sawada]

On Fri, Oct 22, 2021 at 2:25 PM Amit Kapila  wrote:
>
> On Thu, Oct 21, 2021 at 6:47 PM vignesh C  wrote:
> >
> >
> > Thanks for the comments, the attached v45 patch has the fix for the same.
> >
>
> The first patch is mostly looking good to me apart from the below
> minor comments:

Let me share other minor comments on v45-0001 patch:

>
> 1.
> +  
> +   The catalog pg_publication_namespace contains the
> +   mapping between schemas and publications in the database.  This is a
> +   many-to-many mapping.
>
> There are extra spaces after mapping at the end which are not required.

+   ADD and DROP  clauses will add and
+   remove one or more tables/schemas from the publication.  Note that adding
+   tables/schemas to a publication that is already subscribed to will require a

There is also an extra space after "adding".

-[ FOR TABLE [ ONLY ] table_name [ * ] [, ...]
-  | FOR ALL TABLES ]
+[ FOR ALL TABLES
+  | FOR publication_object [, ... ] ]

Similarly, after "TABLES".

+
+ 
+  Specifying a table that is part of a schema specified by
+  FOR ALL TABLES IN SCHEMA is not supported.
+ 

And, after "by".

---

+static void
+AlterPublicationSchemas(AlterPublicationStmt *stmt,
+HeapTuple tup, List
*schemaidlist)
+{
(snip)
+PublicationAddSchemas(pubform->oid, schemaidlist, true, stmt);
+}
+
+return;
+}

The "return" at the end of the function is not necessary.

---
+if (pubobj->name)
+pubobj->pubobjtype =
PUBLICATIONOBJ_REL_IN_SCHEMA;
+else if (!pubobj->name && !pubobj->pubtable)
+pubobj->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
+else if (!pubobj->name)
+ereport(ERROR,
+errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("invalid
schema name at or near"),
+
parser_errposition(pubobj->location));

I think it's better to change the last "else if" to just "else".

---
+
+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, schemarelids);
+}
+else
+tables = relids;
+
+}

There is an extra newline after "table = relids;".

The rest looks good to me.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Added schema level support for publication.

[Greg Nancarrow]

On Fri, Oct 22, 2021 at 12:41 PM Greg Nancarrow  wrote:
>
> I was also previously concerned about what the behavior should be when
> only including just the partitions of a partitioned table in a
> publication using ALL TABLES IN SCHEMA and having
> publish_via_partition_root=true. It seems to implicitly include the
> partitioned table in the publication. But I did some testing and found
> that this is the current behavior when only the partitions are
> individually included in a publication using TABLE, so it seems to be
> OK.
>

Thinking some more about this, it still may still be confusing to the
user if not explicitly stated in the ALL TABLES IN SCHEMA case.
How about adding some additional explanation to the end of the
following paragraph:

+ 
+  When a partitioned table is published via schema level publication, all
+  of its existing and future partitions irrespective of it being from the
+  publication schema or not are implicitly considered to be part of the
+  publication. So, even operations that are performed directly on a
+  partition are also published via publications that its ancestors are
+  part of.
+ 

Something like:

Similarly, if a partition is published via schema level publication
and publish_via_partition_root=true, the parent partitioned table is
implicitly considered to be part of the publication, irrespective of
it being from the publication schema or not.


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

On Thu, Oct 21, 2021 at 6:47 PM vignesh C  wrote:
>
>
> Thanks for the comments, the attached v45 patch has the fix for the same.
>

The first patch is mostly looking good to me apart from the below
minor comments:

1.
+  
+   The catalog pg_publication_namespace contains the
+   mapping between schemas and publications in the database.  This is a
+   many-to-many mapping.

There are extra spaces after mapping at the end which are not required.

2.
+   CREATE privilege on the database.  Also, the new owner
+   of a FOR ALL TABLES publication must be a superuser.

I think we can modify the second line as: "Also, the new owner of a
FOR ALL TABLES or FOR ALL TABLES IN
SCHEMA publication must be a superuser.

3.
/table's schema as part of specified schema is not supported./table's
schema as part of the specified schema is not supported.

4.
+  
+   Create a publication that publishes all changes for tables
+   users, departments and
+   that publishes all changes for all the tables present in the schema
+   production:

I don't think '...that publishes...' is required twice in the above sentence.

5.
+static List *OpenReliIdList(List *relids);
 static List *OpenTableList(List *tables);
 static void CloseTableList(List *rels);
 static void PublicationAddTables(Oid pubid, List *rels, bool if_not_exists,
  AlterPublicationStmt *stmt);
 static void PublicationDropTables(Oid pubid, List *rels, bool missing_ok);
+static void LockSchemaList(List *schemalist);
+static void PublicationAddSchemas(Oid pubid, List *schemas, bool if_not_exists,
+   AlterPublicationStmt *stmt);
+static void PublicationDropSchemas(Oid pubid, List *schemas, bool missing_ok);

Keep the later definitions also in this order. I suggest move
LockSchemaList() just after CloseTableList() both in declaration and
definition.

6.
+/*
+ * Convert the PublicationObjSpecType list into schema oid list and rangevar
+ * list.
+ */

I think you need to say publication table instead of rangevar in the
above comment.

7.
+ /*
+ * It is quite possible that for the SET case user has not specified any
+ * schema in which case we need to remove all the existing schemas.
+ */

/schema/schemas

8.
+/*
+ * Open relations specified by a RangeVar list.

/RangeVar/PublicationTable

9.
+static bool
+_equalPublicationObject(const PublicationObjSpec *a,
+ const PublicationObjSpec *b)
+{
+ COMPARE_SCALAR_FIELD(pubobjtype);
+ COMPARE_STRING_FIELD(name);
+ COMPARE_NODE_FIELD(pubtable);
+ COMPARE_LOCATION_FIELD(location);
+
+ return true;
+}
+

Let's define this immediately before _equalPublicationTable as all
publication functions are defined there. Also, make the handling of
T_PublicationObjSpec before T_PublicationTable in equal() function as
that is the way nodes are defined.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Greg Nancarrow]

On Fri, Oct 22, 2021 at 12:19 AM vignesh C  wrote:
>
> 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
>

Thanks for addressing this.
I checked the updated code and did some more testing and the fix LGTM.

I was also previously concerned about what the behavior should be when
only including just the partitions of a partitioned table in a
publication using ALL TABLES IN SCHEMA and having
publish_via_partition_root=true. It seems to implicitly include the
partitioned table in the publication. But I did some testing and found
that this is the current behavior when only the partitions are
individually included in a publication using TABLE, so it seems to be
OK.

Regards,
Greg Nancarrow
Fujitsu Australia

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.

[Greg Nancarrow]

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).

Regards,
Greg Nancarrow
Fujitsu Australia

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Thurs, Oct 21, 2021 12:25 AM vignesh C  wrote:
> This version of patch retains the changes related to PublicationRelInfo, I 
> will
> handle the merging of the patches in the next version so that this version of
> patch change related to PublicationRelInfo can be reviewed easily.

Thanks for the patches,
I think the change related to PublicationRelInfo looks good.

Best regards,
Hou zj

Re: Added schema level support for publication.

[Greg Nancarrow]

On Thu, Oct 21, 2021 at 3:25 AM vignesh C  wrote:
>
> Attached v44 patch as the fixes for the same.
>

Regarding the documentation, I think some minor updates are needed in:
 doc/src/sgml/logical-replication.sgml.
For example, currently it says:
   Publications may currently only contain tables. Objects must be
added explicitly, except when a publication is created for
ALL TABLES.
There is also some security-related information in this file which may
need updating for ALL TABLES IN SCHEMA.

Also, I'm not sure the documentation updates in the patches clearly
define how partitions relate to ALL TABLES IN SCHEMA.
For example, if a partitioned table belongs to a different schema to
that of a child partition that belongs to a schema specified for ALL
TABLES IN SCHEMA, is it implicitly included in the publication or not
included?

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

On Tue, Oct 19, 2021 at 9:42 PM vignesh C  wrote:
>

Thanks for the modified patch. I have a few more comments and suggestions:

As the thread [1] is still not concluded, I suggest we fix the
duplicate data case only when schemas are involved by slightly
tweaking the code as per attached. This is just to give you an idea
about what I have in mind, if you find a better solution then feel
free to let me know.

Few additional minor comments:
1.
+-- Test the list of partitions published

Shall we change the above comment as: "Test the list of partitions
published with or without 'PUBLISH_VIA_PARTITION_ROOT' parameter"?

2. psql documentation for \dRp[+] needs to be modified.

\dRp
Before:
" .. If + is appended to the command name, the tables associated with
each publication are shown as well."
After:
" .. If + is appended to the command name, the tables and schemas
associated with each publication are shown as well.

Apart from the above, I think we should merge the first four patches
as there doesn't seem to be any big problems pending. We can keep
still keep tests added by 025_rep_changes_for_schema.pl as a separate
patch as there might be some timing-dependent tests in that file.

[1] - 
https://www.postgresql.org/message-id/OS0PR01MB57167F45D481F78CDC5986F794B99%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.


change_partitions_schema_interaction_1.patch
Description: Binary data

RE: Added schema level support for publication.

[tanghy.f...@fujitsu.com]

On Tuesday, October 19, 2021 11:42 PM vignesh C  wrote:
> 
> This issue got induced in the v42 version, attached v43 patch has the
> fixes for the same.
> 

Thanks for your new patch. I confirmed that this issue has be fixed.

All regression tests passed. 
I also tested V43 in some other scenarios and found no issue.
So the v43 patch LGTM.

Regards
Tang

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.

[Amit Kapila]

On Mon, Oct 18, 2021 at 5:53 PM vignesh C  wrote:
>

Few comments on latest set of patches:
===
1.
+/*
+ * Filter out the partitions whose parent tables was also specified in
+ * the publication.
+ */
+static List *
+filter_out_partitions(List *relids)

Can we name this function as filter_partitions()?

2.
+ /*
+ * 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 the data of child table double-published in
+ * subscriber side.
+ */

Let's slightly change the last part of the line in the above comment
as: "... which could cause data of the child table to be
double-published on the subscriber side."

3.
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
..
..
@@ -38,7 +40,6 @@
 #include "utils/builtins.h"
 #include "utils/catcache.h"
 #include "utils/fmgroids.h"
-#include "utils/inval.h"
 #include "utils/lsyscache.h"

Does this change belong to this patch? If not, maybe you can submit a
separate patch for this. A similar change is present in
publicationcmds.c as well, not sure if that is required as well.

4.
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
...
...
+#include "nodes/makefuncs.h"

Do we need to include this file? I am able to compile without
including this file.

v42-0003-Add-tests-for-the-schema-publication-feature-of-
5.
+-- pg_publication_tables
+SET client_min_messages = 'ERROR';
+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 PUBLICATION pub FOR ALL TABLES IN SCHEMA sch2 WITH
(PUBLISH_VIA_PARTITION_ROOT=1);
+SELECT * FROM pg_publication_tables;
+
+DROP PUBLICATION pub;
+CREATE PUBLICATION pub FOR TABLE sch2.tbl1_part1 WITH
(PUBLISH_VIA_PARTITION_ROOT=1);
+SELECT * FROM pg_publication_tables;

Can we expand the above comment on the lines of: "Test the list of
partitions published"?

v42-0004-Add-documentation-for-the-schema-publication-fea
6.
+ 
+  pg_publication_namespace
+  schema to publication mapping
+ 
+
  
   pg_publication_rel
   relation to publication mapping
@@ -6238,6 +6243,67 @@ SCRAM-SHA-256$iteration
count:
   
  

+ 
+  pg_publication_namespace

At one place, the new catalog is placed after pg_publication_rel and
at another place, it is before it. Shouldn't it be before in both
places as we have a place as per naming order?

7.
The
+   ADD clause will add one or more tables/schemas to the
+   publication. The DROP clauses will remove one or more
+   tables/schemas from the publication.

Isn't it better to write the above as one line: "The
ADD and DROP clauses will add
and remove one or more tables/schemas from the publication."?

8.
+  
+   Add some schemas to the publication:
+
+ALTER PUBLICATION sales_publication ADD ALL TABLES IN SCHEMA
marketing_june, sales_june;
+
+  

Can we change schema names to just marketing and sales? Also, let's
change the description as:"Add schemas
marketing and sales
to the publication sales_publication?

9.
+[ FOR ALL TABLES
+  | FOR publication
object [, ... ] ]
 [ WITH ( publication_parameter [= value] [, ... ] ) ]
+
+where publication
object is one of:

Similar to Alter Publication, here also we should use
publication_object instead of publication object.

10.
+  
+   Create a publication that publishes all changes for tables "users" and
+   "departments" and that publishes all changes for all the tables present in
+   the schema "production":
+
+CREATE PUBLICATION production_publication FOR TABLE users,
departments, ALL TABLES IN SCHEMA production;
+
+  
+
+  
+   Create a publication that publishes all changes for all the tables
present in
+   the schemas "marketing" and "sales":

It is better to use  before and  after schema
names in above descriptions.

-- 
With Regards,
Amit Kapila.

RE: Added schema level support for publication.

[tanghy.f...@fujitsu.com]

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.

Regards
Tang


Topup-permissions-test_diff
Description: Topup-permissions-test_diff

Re: Added schema level support for publication.

[Amit Kapila]

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?

-- 
With Regards,
Amit Kapila.

RE: Added schema level support for publication.

[tanghy.f...@fujitsu.com]

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.

@@ -165,6 +265,12 @@ CreatePublication(ParseState *pstate, 
CreatePublicationStmt *stmt)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 errmsg("must be superuser to create FOR ALL 
TABLES publication")));
 
+   /* FOR ALL TABLES IN SCHEMA requires superuser */
+   if (list_length(schemaidlist) > 0 && !superuser())
+   ereport(ERROR,
+   errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+   errmsg("must be superuser to create FOR ALL 
TABLES IN SCHEMA publication"));
+
rel = table_open(PublicationRelationId, RowExclusiveLock);
 
/* Check if name is used */

Regards
Tang

Re: Added schema level support for publication.

[Masahiko Sawada]

Hi,

On Mon, Oct 18, 2021 at 3:14 PM houzj.f...@fujitsu.com
 wrote:
>
> On Saturday, October 16, 2021 1:57 PM houzj.f...@fujitsu.com wrote:
> > Based on the V40 patchset, attaching the Top-up patch which try to fix the
> > partition issue in a cleaner way.
>
> Attach the new version patch set which merge the partition fix into it.
> Besides, instead of introducing new function and parameter, just add the
> partition filter in pg_get_publication_tables which makes the code cleaner.
>
> Only 0001 and 0003 was changed.

I've reviewed 0001 and 0002 patch and here are comments:

0001 patch:

+/*
+ * Get the list of publishable relation oids for a specified schema.
+ *
+ * Schema will be having both ordinary('r') relkind tables and partitioned('p')
+ * relkind tables, so two rounds of scan are required.
+ */
+List *
+GetSchemaPublicationRelations(Oid schemaid, PublicationPartOpt pub_partopt)
+{
+Relation   classRel;
+ScanKeyData key[3];
+TableScanDesc scan;

I think it's enough to have key[2], not key[3].

BTW, this function does the table scan on pg_class twice in order to
get OIDs of both normal tables and partitioned tables. But can't we do
that by the single table scan? I think we can set a scan key for
relnamespace, and check relkind inside a scan loop.

---
+ObjectsInPublicationToOids(stmt->pubobjects, pstate,
,
+
);
+
+if (list_length(relations) > 0)
+{
+List  *rels;
+
+rels = OpenTableList(relations);
+CheckObjSchemaNotAlreadyInPublication(rels,
schemaidlist,
+
PUBLICATIONOBJ_TABLE);
+PublicationAddTables(puboid, rels, true, NULL);
+CloseTableList(rels);
+}
+
+if (list_length(schemaidlist) > 0)
+{
+/* FOR ALL TABLES IN SCHEMA requires superuser */
+if (!superuser())
+ereport(ERROR,
+
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+errmsg("must be
superuser to create FOR ALL TABLES IN SCHEMA publication"));
+

Perhaps we can do a superuser check before handling "relations"? If
the user doesn't have the permission, we don't need to do anything for
relations.

0002 patch:

postgres(1:13619)=# create publication pub for all TABLES in schema
CURRENT_SCHEMA  pg_catalog  public  s2
information_schema  pg_toasts1

Since pg_catalog and pg_toast cannot be added to the schema
publication can we exclude them from the completion list?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Monday, October 18, 2021 12:04 PM Greg Nancarrow wrote:
> On Sat, Oct 16, 2021 at 4:57 PM houzj.f...@fujitsu.com wrote:
> > Based on the V40 patchset, attaching the Top-up patch which try to fix
> > the partition issue in a cleaner way.
> >
> 
> A minor thing, in your "top-up patch", the test code added to publication.sql,
> you need to remove the last "DROP TABLE sch2.tbl1_part1;". It causes an error
> because the table doesn't exist and it seems to have been erroneously copied
> from the previous test case.

Thanks for the comment.
I have removed the last "DROP TABLE sch2.tbl1_part1;" in V41 patch set.

Best regards,
Hou zj

Re: Added schema level support for publication.

[Greg Nancarrow]

On Sat, Oct 16, 2021 at 4:57 PM houzj.f...@fujitsu.com
 wrote:
>
> Besides, I found we misunderstood the flag PUBLICATION_PART_ROOT it means:
> "ROOT: only the table explicitly mentioned in the publication" We cannot use 
> it
> as a flag to judge whether do the partition filtering, I think we need to pass
> the actual pubviaroot flag.
>

I agree, PUBLICATION_PART_ROOT can't be used to determine whether to
do partition filtering, the "pubviaroot" flag is needed.

> Based on the V40 patchset, attaching the Top-up patch which try to fix the
> partition issue in a cleaner way.
>

A minor thing, in your "top-up patch", the test code added to
publication.sql, you need to remove the last "DROP TABLE
sch2.tbl1_part1;". It causes an error because the table doesn't exist
and it seems to have been erroneously copied from the previous test
case.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

On Fri, Oct 15, 2021 at 6:45 AM Greg Nancarrow  wrote:
>
> On Thu, Oct 14, 2021 at 9:59 PM Amit Kapila  wrote:
> >
> > > If partitions belong to a different schema than the parent partitioned
> > > table, then the current patch implementation allows the partitions to
> > > (optionally) be explicitly added to a publication that includes the
> > > parent partitioned table (and for the most part, it doesn't seem to
> > > make any difference to the publication behavior). Should this be
> > > allowed?
> > >
> > > e.g.
> > >
> > > CREATE SCHEMA sch;
> > > CREATE SCHEMA sch1;
> > > CREATE TABLE sch.sale (sale_date date not null, country_code text,
> > > product_sku text, units integer) PARTITION BY RANGE (sale_date);
> > > CREATE TABLE sch1.sale_201901 PARTITION OF sch.sale FOR VALUES FROM
> > > ('2019-01-01') TO ('2019-02-01');
> > > CREATE TABLE sch1.sale_201902 PARTITION OF sch.sale FOR VALUES FROM
> > > ('2019-02-01') TO ('2019-03-01');
> > >
> > > postgres=# CREATE PUBLICATION pub FOR ALL TABLES IN SCHEMA sch, TABLE
> > > sch1.sale_201901, TABLE sch1.sale_201902;
> > > CREATE PUBLICATION
> > > postgres=# \dRp+
> > >  Publication pub
> > >  Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
> > > ---++-+-+-+---+--
> > >  gregn | f  | t   | t   | t   | t | f
> > > Tables:
> > > "sch1.sale_201901"
> > > "sch1.sale_201902"
> > > Tables from schemas:
> > > "sch"
> > >
> >
> > I don't see any problem with this. Do you have a specific problem in
> > mind due to this?
> >
>
> I'm not sure if it's a problem as such, really just a query from me as
> to whether it should be allowed to also (redundantly) add partitions
> to the publication, in addition to the partitioned table, since the
> current documentation says: "When a partitioned table is added to a
> publication, all of its existing and future partitions are implicitly
> considered to be part of the publication".
> I guess it should be allowed, as I find I can do it in the current
> implementation just with TABLE.
>

I have also checked the "For Table" case and it behaves similar to
what the patch has for schema. So, I think it is better to retain the
current behavior of patch.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Greg Nancarrow]

On Thu, Oct 14, 2021 at 9:59 PM Amit Kapila  wrote:
>
> > If partitions belong to a different schema than the parent partitioned
> > table, then the current patch implementation allows the partitions to
> > (optionally) be explicitly added to a publication that includes the
> > parent partitioned table (and for the most part, it doesn't seem to
> > make any difference to the publication behavior). Should this be
> > allowed?
> >
> > e.g.
> >
> > CREATE SCHEMA sch;
> > CREATE SCHEMA sch1;
> > CREATE TABLE sch.sale (sale_date date not null, country_code text,
> > product_sku text, units integer) PARTITION BY RANGE (sale_date);
> > CREATE TABLE sch1.sale_201901 PARTITION OF sch.sale FOR VALUES FROM
> > ('2019-01-01') TO ('2019-02-01');
> > CREATE TABLE sch1.sale_201902 PARTITION OF sch.sale FOR VALUES FROM
> > ('2019-02-01') TO ('2019-03-01');
> >
> > postgres=# CREATE PUBLICATION pub FOR ALL TABLES IN SCHEMA sch, TABLE
> > sch1.sale_201901, TABLE sch1.sale_201902;
> > CREATE PUBLICATION
> > postgres=# \dRp+
> >  Publication pub
> >  Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
> > ---++-+-+-+---+--
> >  gregn | f  | t   | t   | t   | t | f
> > Tables:
> > "sch1.sale_201901"
> > "sch1.sale_201902"
> > Tables from schemas:
> > "sch"
> >
>
> I don't see any problem with this. Do you have a specific problem in
> mind due to this?
>

I'm not sure if it's a problem as such, really just a query from me as
to whether it should be allowed to also (redundantly) add partitions
to the publication, in addition to the partitioned table, since the
current documentation says: "When a partitioned table is added to a
publication, all of its existing and future partitions are implicitly
considered to be part of the publication".
I guess it should be allowed, as I find I can do it in the current
implementation just with TABLE.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

On Wed, Oct 13, 2021 at 1:40 PM Greg Nancarrow  wrote:
>
> On Wed, Oct 13, 2021 at 12:15 AM vignesh C  wrote:
> >
> > Attached v40 patch has the fix for the above comments.
> >
>
> [Maybe this has some overlap with what Hou-san reported, and I have
> not tested this against his proposed fixes]
>
> If partitions belong to a different schema than the parent partitioned
> table, then the current patch implementation allows the partitions to
> (optionally) be explicitly added to a publication that includes the
> parent partitioned table (and for the most part, it doesn't seem to
> make any difference to the publication behavior). Should this be
> allowed?
>
> e.g.
>
> CREATE SCHEMA sch;
> CREATE SCHEMA sch1;
> CREATE TABLE sch.sale (sale_date date not null, country_code text,
> product_sku text, units integer) PARTITION BY RANGE (sale_date);
> CREATE TABLE sch1.sale_201901 PARTITION OF sch.sale FOR VALUES FROM
> ('2019-01-01') TO ('2019-02-01');
> CREATE TABLE sch1.sale_201902 PARTITION OF sch.sale FOR VALUES FROM
> ('2019-02-01') TO ('2019-03-01');
>
> postgres=# CREATE PUBLICATION pub FOR ALL TABLES IN SCHEMA sch, TABLE
> sch1.sale_201901, TABLE sch1.sale_201902;
> CREATE PUBLICATION
> postgres=# \dRp+
>  Publication pub
>  Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
> ---++-+-+-+---+--
>  gregn | f  | t   | t   | t   | t | f
> Tables:
> "sch1.sale_201901"
> "sch1.sale_201902"
> Tables from schemas:
> "sch"
>

I don't see any problem with this. Do you have a specific problem in
mind due to this?

-- 
With Regards,
Amit Kapila.

RE: Added schema level support for publication.

[tanghy.f...@fujitsu.com]

On Wednesday, October 13, 2021 4:10 PM Greg Nancarrow  
wrote:
> Also, I found the following scenario where the data is double-published:
> 
> (1) PUB:  CREATE PUBLICATION pub FOR TABLE sch1.sale_201901, TABLE
> sch1.sale_201902 WITH (publish_via_partition_root=true);
> (2) SUB:  CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres
> host=localhost port=5432' PUBLICATION pub;
> (3) PUB:  INSERT INTO sch.sale VALUES('2019-01-01', 'AU', 'cpu', 5),
> ('2019-01-02', 'AU', 'disk', 8);
> (4) SUB:  SELECT * FROM sch.sale;
> (5) PUB:  ALTER PUBLICATION pub ADD ALL TABLES IN SCHEMA sch;
> (6) SUB:  ALTER SUBSCRIPTION sub REFRESH PUBLICATION;
> (7) SUB:  SELECT * FROM sch.sale;
> 
> sale_date  | country_code | product_sku | units
> +--+-+---
>  2019-01-01 | AU   | cpu | 5
>  2019-01-02 | AU   | disk| 8
>  2019-01-01 | AU   | cpu | 5
>  2019-01-02 | AU   | disk| 8
> 
> 

I changed your test step in (5) and used "ADD TABLE" command as below:
ALTER PUBLICATION pub ADD TABLE sch.sale;

I could get the same result on HEAD.
So I think it's not a problem related to this patch.
Maybe we can post this issue in a new thread.

Regards
Tang

Re: Added schema level support for publication.

[Greg Nancarrow]

On Wed, Oct 13, 2021 at 12:15 AM vignesh C  wrote:
>
> Attached v40 patch has the fix for the above comments.
>

[Maybe this has some overlap with what Hou-san reported, and I have
not tested this against his proposed fixes]

If partitions belong to a different schema than the parent partitioned
table, then the current patch implementation allows the partitions to
(optionally) be explicitly added to a publication that includes the
parent partitioned table (and for the most part, it doesn't seem to
make any difference to the publication behavior). Should this be
allowed?

e.g.

CREATE SCHEMA sch;
CREATE SCHEMA sch1;
CREATE TABLE sch.sale (sale_date date not null, country_code text,
product_sku text, units integer) PARTITION BY RANGE (sale_date);
CREATE TABLE sch1.sale_201901 PARTITION OF sch.sale FOR VALUES FROM
('2019-01-01') TO ('2019-02-01');
CREATE TABLE sch1.sale_201902 PARTITION OF sch.sale FOR VALUES FROM
('2019-02-01') TO ('2019-03-01');

postgres=# CREATE PUBLICATION pub FOR ALL TABLES IN SCHEMA sch, TABLE
sch1.sale_201901, TABLE sch1.sale_201902;
CREATE PUBLICATION
postgres=# \dRp+
 Publication pub
 Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
---++-+-+-+---+--
 gregn | f  | t   | t   | t   | t | f
Tables:
"sch1.sale_201901"
"sch1.sale_201902"
Tables from schemas:
"sch"


Also, I found the following scenario where the data is double-published:

(1) PUB:  CREATE PUBLICATION pub FOR TABLE sch1.sale_201901, TABLE
sch1.sale_201902 WITH (publish_via_partition_root=true);
(2) SUB:  CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres
host=localhost port=5432' PUBLICATION pub;
(3) PUB:  INSERT INTO sch.sale VALUES('2019-01-01', 'AU', 'cpu', 5),
('2019-01-02', 'AU', 'disk', 8);
(4) SUB:  SELECT * FROM sch.sale;
(5) PUB:  ALTER PUBLICATION pub ADD ALL TABLES IN SCHEMA sch;
(6) SUB:  ALTER SUBSCRIPTION sub REFRESH PUBLICATION;
(7) SUB:  SELECT * FROM sch.sale;

sale_date  | country_code | product_sku | units
+--+-+---
 2019-01-01 | AU   | cpu | 5
 2019-01-02 | AU   | disk| 8
 2019-01-01 | AU   | cpu | 5
 2019-01-02 | AU   | disk| 8


Regards,
Greg Nancarrow
Fujitsu Australia

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Tuesday, October 12, 2021 9:15 PM vignesh C 
> Attached v40 patch has the fix for the above comments.

Thanks for the update, I have some minor issues about partition related 
behavior.

1)

Tang tested and discussed this issue with me.
The testcase is:
We publish a schema and there is a partition in the published schema. If
publish_via_partition_root is on and the partition's parent table is not in the
published schema, neither the change on the partition nor the parent table will
not be published.

But if we publish by FOR TABLE partition and set publish_via_partition_root to
on, the change on the partition will be published. So, I think it'd be better to
publish the change on partition for FOR ALL TABLES IN SCHEMA case if its parent 
table
is not published in the same publication.

It seems we should pass publication oid to the GetSchemaPublicationRelations()
and add some check like the following:

if (is_publishable_class(relid, relForm) &&
!(relForm->relispartition && pub_partopt == 
PUBLICATION_PART_ROOT))
result = lappend_oid(result, relid);
+   if (relForm->relispartition && pub_partopt == 
PUBLICATION_PART_ROOT)
+   {
+   bool skip = false;
+   List *ancestors = get_partition_ancestors(relid);
+   List *schemas = GetPublicationSchemas(pubid);
+   ListCell   *lc;
+   foreach(lc, ancestors)
+   {
+   if (list_member_oid(schemas, 
get_rel_namespace(lfirst_oid(lc
+   {
+   skip = true;
+   break;
+   }
+   }
+   if (!skip)
+   result = lappend_oid(result, relid);
+   }



2)
+   /*
+* It is quite possible that some of the partitions are in a different
+* schema than the parent table, so we need to get such partitions
+* separately.
+*/
+   scan = table_beginscan_catalog(classRel, keycount, key);
+   while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+   {
+   Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
+
+   if (is_publishable_class(relForm->oid, relForm))
+   {
+   List   *partitionrels = NIL;
+
+   partitionrels = 
GetPubPartitionOptionRelations(partitionrels,
+   
   pub_partopt,
+   
   relForm->oid);

I think a partitioned table could also be a partition which should not be
appended to the list. I think we should also filter these cases here by same
check in 1).
Thoughts ?

Best regards,
Hou zj

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Monday, October 11, 2021 11:02 PM vignesh C  wrote:
> The attached v39 patch has the fixes for the above issues.

Thanks for the updates.
I have a few minor suggestions about the testcases in the v39-0003-Test patch.

1)
+-- alter publication drop CURRENT_SCHEMA
+ALTER PUBLICATION testpub1_forschema DROP ALL TABLES IN SCHEMA CURRENT_SCHEMA;
+\dRp+ testpub1_forschema

Since we already tested CURRENT_SCHEMA in various CREATE PUBLICATION cases, 
maybe
we don't need to test it again in SET/DROP/ADD cases.

2)
+-- alter publication set schema
+ALTER PUBLICATION testpub1_forschema SET ALL TABLES IN SCHEMA pub_test1;
+\dRp+ testpub1_forschema
+
+-- alter publication set multiple schema
+ALTER PUBLICATION testpub1_forschema SET ALL TABLES IN SCHEMA pub_test1, 
pub_test2;
+\dRp+ testpub1_forschema
+

I think the multiple schemas testcase is sufficient, maybe we can remove the
single schema case.


3)
+
+-- alter publication set it with the same schema
+ALTER PUBLICATION testpub1_forschema SET ALL TABLES IN SCHEMA pub_test1, 
pub_test2;
+\dRp+ testpub1_forschema

ISTM, we didn't have some special code path for this case, maybe we can remove
this testcase.


Best regards,
Hou zj

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.

[Greg Nancarrow]

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).

Regards,
Greg Nancarrow
Fujitsu Australia

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Monday, October 11, 2021 2:39 PM vignesh C  wrote:
> 
> These comments are fixed in the v38 patch attached.

Thanks for updating the patches.
Here are a few comments on the v38-0004-Doc patch.

1.
+  
+   Adding/Setting a table that is part of schema specified in
+   ALL TABLES IN SCHEMA, adding/setting a schema to a
+   publication along with same schema's table specified with
+   TABLE, adding/setting a schema to a publication that
+   already has a table that is part of specified schema or adding/setting a
+   table to a publication that already has a table's schema as part of
+   specified schema is not supported.

ISTM we can remove the description "adding/setting a schema to a publication
along with same schema's table specified with TABLE",
because it seems the same as the first mentioned case "Adding/Setting a table
that is part of schema specified in ALL TABLES IN SCHEMA"

2.

+
+
+  
+   Add some schemas to the publication:
+
+ALTER PUBLICATION sales_publication ADD ALL TABLES IN SCHEMA marketing_june, 
sales_june;
+
+  
+
+  
+   Add some tables and schemas to the publication:
...
+
+  
+   Drop some schemas from the publication:
...
+  
+   Set some schemas to the publication:
+
+ALTER PUBLICATION production_publication SET ALL TABLES IN SCHEMA 
production_september, production_october;

Personally, I think we don't need the example about DROP and SET here.
The example of ADD seems sufficient.

3.
+
+  
+
+  
+   Create a publication that publishes all changes for all the tables present 
in
+   the schema "production":
+
+CREATE PUBLICATION production_publication FOR ALL TABLES IN SCHEMA production;
+
+  
...
+  
+   Create a publication that publishes all changes for all the tables present 
in
+   the schemas "marketing" and "sales":
+
+CREATE PUBLICATION sales_publication FOR ALL TABLES IN SCHEMA marketing, sales;

I think the example for publishing all the tables in schemas "marketing" and
"sales" is sufficient, the example for pulishing signal schema seems can be
removed.

Best regards,
Hou zj

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 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 ADD 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 updating the table, set REPLICA IDENTITY using ALTER TABLE.
>
> I think here we don't need to test both SET and ADD variants, one of
> them is 

RE: Added schema level support for publication.

[tanghy.f...@fujitsu.com]

> 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.


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");


Regards
Tang

Re: Added schema level support for publication.

[Amit Kapila]

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 (");

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 (

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?

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'.

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.

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 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 ADD 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 updating the table, set REPLICA IDENTITY using ALTER TABLE.

I think here we don't need to test both SET and ADD variants, one of
them is sufficient.

7.
+-- verify invalidation of partition table having partition on different schema

I think this comment is not very clear to me. Can we change it to:
"verify invalidation of partition table having parent and child tables
in different schema"?

8.
+-- verify invalidation of schema having partition parent table and
partition child table

Similarly, let's change this to: "verify invalidation of partition
tables for schema publication that has parent and child tables of
different partition hierarchies". Keep comments line boundary as 80
chars, that way they look readable.

9.
+++ b/src/test/subscription/t/025_rep_changes_for_schema.pl
..
+# Basic logical replication test

Let's change this comment to: "Logical replication tests for schema
publications"


-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

On Wed, Oct 6, 2021 at 11:12 AM vignesh C  wrote:
>
> Attached v37 patch has the changes for the same.
>

Few comments:
==
v37-0001-Added-schema-level-support-for-publication
1.
+ *
+ * The first scan will get all the 'r' relkind tables for the specified schema,
+ * iterate the 'r' relkind tables and prepare a list of:
+ * 1) non partition table if pub_partopt is PUBLICATION_PART_ROOT
+ * 2) partition table and non partition table if pub_partopt is
+ *PUBLICATION_PART_LEAF.
+ *
+ * The second scan will get all the 'p'' relkind tables for the  specified
+ * schema, iterate the 'p' relkind tables and prepare a list of:
+ * 1) partition table's child relations if pub_partopt is PUBLICATION_PART_LEAF
+ * 2) partition table if pub_partopt is PUBLICATION_PART_ROOT.

I think these comments are redundant and not sure if they are
completely correct. We don't need these as the actual code explains
these conditions better. The earlier part of these comments is
sufficient.

v37-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN
2.
+ * selectDumpablePublicationObject: policy-setting subroutine
+ * Mark a publication as to be dumped or not
  *
- * Publication tables have schemas, but those are ignored in decision making,
+ * Publications have schemas, but those are ignored in decision making,
  * because publications are only dumped when we are dumping everything.
  */

Change the above comment lines:
a. "Mark a publication as to be dumped or not" to "Mark a publication
object as to be dumped or not".

b. "Publications have schemas, but those are ignored in decision
making, .." to "A publication can have schemas and tables which have
schemas, but those are ignored in decision making, .."

3.
+/*
+ * dumpPublicationNamespace
+ *   dump the definition of the given publication tables in schema mapping
+ */

Can we change the comment to: "dump the definition of the given
publication schema mapping"? IT is easier to read and understand.

4.
+/*
+ * The PublicationSchemaInfo struct is used to represent publication tables
+ * in schema mapping.
+ */
+typedef struct _PublicationSchemaInfo
+{
+ DumpableObject dobj;
+ NamespaceInfo *pubschema;
+ PublicationInfo *publication;
+} PublicationSchemaInfo;

Can we change the comment similar to the comment change in point 3?
Also, let's keep PublicationInfo * before NamespaceInfo * just to be
consistent with the existing structure PublicationRelInfo?

5.
+ printfPQExpBuffer(,
+   "SELECT p.pubname\n"
+   "FROM pg_catalog.pg_publication p\n"
+   " JOIN pg_catalog.pg_publication_namespace pn ON p.oid = pn.pnpubid\n"
+   " JOIN pg_catalog.pg_class pc ON pc.relnamespace = pn.pnnspid AND
pc.oid = '%s'\n"

I think this part of the query can be improved in multiple ways: (a)
pc.oid = '%s' should be part of WHERE clause not join condition, (b)
for pubname, no need to use alias name, it can be directly referred as
pubname, (c) you can check if the relation is publishable. So, the
formed query would look like:

SELECT p.pubname FROM pg_catalog.pg_publication p JOIN
pg_catalog.pg_publication_namespace pn ON p.oid = pn.pnpubid JOIN
pg_catalog.pg_class pc ON pc.relnamespace = pn.pnnspid  WHERE pc.oid =
'%s' and pg_catalog.pg_relation_is_publishable('%s')

6.
listSchemas()
{
..
+ if (pub_schema_tuples > 0)
+ {
+ /*
+ * Allocate memory for footers. Size of footers will be 1 (for
+ * storing "Publications:" string) + Schema count +  1 (for
+ * storing NULL).
+ */
+ footers = (char **) palloc((1 + pub_schema_tuples + 1) * sizeof(char *));
+ footers[0] = pstrdup(_("Publications:"));
+
+ /* Might be an empty set - that's ok */
+ for (i = 0; i < pub_schema_tuples; i++)
+ {
+ printfPQExpBuffer(, "\"%s\"",
+   PQgetvalue(result, i, 0));
+
+ footers[i + 1] = pstrdup(buf.data);
+ }
+
+ footers[i + 1] = NULL;
+ myopt.footers = footers;
+ }
..
}

Is there a reason of not using printTableAddFooter() here similar to
how we use it to print Publications in describeOneTableDetails()?

7.
describePublications()
{
..
+ /* Get the schemas for the specified publication */
+ printfPQExpBuffer(,
+   "SELECT n.nspname\n"
+   "FROM pg_catalog.pg_namespace n,\n"
+   " pg_catalog.pg_publication_namespace pn\n"
+   "WHERE n.oid = pn.pnnspid\n"
+   "  AND pn.pnpubid = '%s'\n"
+   "ORDER BY 1", pubid);
+ if (!addFooterToPublicationDesc(, "Tables from schemas:",
+ true, ))
+ goto error_return;
..
}

Shouldn't we try to get schemas only when pset.sversion >= 15?

8.
+addFooterToPublicationDesc(PQExpBuffer buf, char *footermsg,
+bool singlecol, printTableContent *cont)
+{
..
+ termPQExpBuffer(buf);
..
}

It seems this buffer is freed at the caller's site, if so, no need to
free it here.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Greg Nancarrow]

On Wed, Oct 6, 2021 at 4:42 PM vignesh C  wrote:
>
> Attached v37 patch has the changes for the same.
>

A small issue I noticed is that using "\dS" in PSQL shows UNLOGGED
tables as belonging to a publication, if the table belongs to a schema
that was added to the publication using ALL TABLES IN SCHEMA (yet
doesn't show as part of an ALL TABLES publication).
Since publication of UNLOGGED tables is silently skipped in the case
of ALL TABLES and ALL TABLES IN SCHEMA, it shouldn't show as belonging
to the publication, right?

test=# \dRp+ pub2
Publication pub2
 Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
---++-+-+-+---+--
 gregn | t  | t   | t   | t   | t | f
(1 row)

test=# \dRp+ pub
 Publication pub
 Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
---++-+-+-+---+--
 gregn | f  | t   | t   | t   | t | t
Tables from schemas:
"sch1"

test=# \dS sch1.test2
Unlogged table "sch1.test2"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 i  | integer |   |  |
Publications:
"pub"


Regards,
Greg Nancarrow
Fujitsu Australia

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 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.

[Amit Kapila]

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.

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Greg Nancarrow]

On Tue, Oct 5, 2021 at 4:40 PM Greg Nancarrow  wrote:
>
>At the
> moment, the existing documentation just states FOR TABLE that
> "Temporary tables, unlogged tables, foreign tables, materialized
> views, and regular views cannot be part of a publication".

Oh, I see that in the v36-0004 doc update patch, it has added the
following for CREATE PUBLICATION ... FOR ALL TABLES IN SCHEMA:
"Only persistent base tables and partitioned tables present in the
schema will be included as part of the publication.  Temporary tables,
unlogged tables, foreign tables, materialized views, and regular views
from the schema will not be part of the publication."


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Greg Nancarrow]

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?
>

OK, it seems that for the ALL TABLES case, there is no such error
check, and it just silently skips replication of any
temporary/unlogged tables. This is intentional, right?
I couldn't see any documentation specifically related to this, so I
think perhaps it should be updated to describe this behaviour. At the
moment, the existing documentation just states FOR TABLE that
"Temporary tables, unlogged tables, foreign tables, materialized
views, and regular views cannot be part of a publication".
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?

With the patch applied:

postgres=# create publication pub3 for all tables in schema sch;
CREATE PUBLICATION
postgres=# create table sch.test3(i int);
CREATE TABLE
postgres=# 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,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

On Tue, Oct 5, 2021 at 6:57 AM Greg Nancarrow  wrote:
>
> (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
>

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?

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Greg Nancarrow]

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 */

(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;

(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

(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


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


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

On Sun, Oct 3, 2021 at 11:25 PM vignesh C  wrote:
>
> On Sat, Oct 2, 2021 at 1:13 PM Amit Kapila  wrote:
> >
>
> > 2. In GetSchemaPublicationRelations(), I think we need to perform a
> > second scan using RELKIND_PARTITIONED_TABLE only if we
> > publish_via_root (aka pub_partopt is PUBLICATION_PART_ROOT). This is
> > what we are doing in GetAllTablesPublicationRelations. Is there a
> > reason to be different here?
>
> In the first table scan we are getting all the ordinary tables present
> in the schema. In the second table scan we will get all the
> partitioned table present in the schema and the relations will be
> added based on pub_partopt. I felt if we have the check we will not
> get the relations in the following case:
> create schema sch1;
> create schema sch2;
> create table sch1.p (a int) partition by list (a);
> create table sch2.c1 partition of sch1.p for values in (1);
>

But we don't need to get the partitioned tables for the invalidations,
see the corresponding case for tables. So, I am not sure why you have
used two scans to the system table for such scenarios?

Few additional comments on
v36-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN:
=
1.
@@ -3961,21 +3965,25 @@ getPublications(Archive *fout, int *numPublications)
  appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
-   "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, p.pubviaroot "
+   "p.puballtables, p.pubinsert, p.pubupdate, "
+   "p.pubdelete, p.pubtruncate, p.pubviaroot "
"FROM pg_publication p",
username_subquery);
  else if (fout->remoteVersion >= 11)
  appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
-   "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, false AS pubviaroot "
+   "p.puballtables, p.pubinsert, p.pubupdate, "
+   "p.pubdelete, p.pubtruncate, false AS pubviaroot "
"FROM pg_publication p",
username_subquery);
  else
  appendPQExpBuffer(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"(%s p.pubowner) AS rolname, "
-   "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, false AS
pubtruncate, false AS pubviaroot "
+   "p.puballtables, p.pubinsert, p.pubupdate, "
+   "p.pubdelete, false AS pubtruncate, "
+   "false AS pubviaroot "
"FROM pg_publication p",
username_subquery);

Is there a reason to change this part of the code?

2.
@@ -257,6 +257,9 @@ getSchemaData(Archive *fout, int *numTablesPtr)
  pg_log_info("reading publication membership");
  getPublicationTables(fout, tblinfo, numTables);

+ pg_log_info("reading publication tables in schemas");
+ getPublicationNamespaces(fout, nspinfo, numNamespaces);

I think for the above change, the first should be changed to "reading
publication membership of tables" and the second one should be changed
to "reading publication membership of schemas".

3. The function names getPublicationNamespaces and
dumpPublicationSchema are not in sync. Let's name the second one as
dumpPublicationNamespace.

4. It is not clear to me why the patch has introduced a new component
type object DUMP_COMPONENT_PUBSCHEMA. In particular, in the below code
if we are already setting DUMP_COMPONENT_ALL, how the additional
setting of DUMP_COMPONENT_PUBSCHEMA helps?

@@ -1631,9 +1631,13 @@ selectDumpableNamespace(NamespaceInfo *nsinfo,
Archive *fout)
  if (nsinfo->nspowner == ROLE_PG_DATABASE_OWNER)
  nsinfo->dobj.dump &= ~DUMP_COMPONENT_DEFINITION;
  nsinfo->dobj.dump_contains = DUMP_COMPONENT_ALL;
+ nsinfo->dobj.dump |= DUMP_COMPONENT_PUBSCHEMA;
  }
  else
+ {
  nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_ALL;
+ nsinfo->dobj.dump |= DUMP_COMPONENT_PUBSCHEMA;
+ }

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

On Thu, Sep 30, 2021 at 3:39 PM vignesh C  wrote:
>
> The suggested change works, I have modified it in the attached patch.
>

I have reviewed the latest version and made a number of changes to the
0001 patch. The changes are in v1-0001-Changes-by-Amit. It includes
(a) Changed preprocess_pubobj_list() to make the code easy to
understand, (b) the handling of few variables was missing in equal
function, (c) the ordering of functions, and a few parameters were not
matching .c and .h files, (d) added/edited multiple comments and other
cosmetic changes.

Apart from that, I have few other comments:
1. It seems you have started using unique list variants in
GetPubPartitionOptionRelations because one of its caller
GetSchemaPublicationRelations need it. I think the unique variants are
costlier, so isn't it better to use it where it is required? I think
it would be good to use in GetPubPartitionOptionRelations, if most of
its caller requires the same.

2. In GetSchemaPublicationRelations(), I think we need to perform a
second scan using RELKIND_PARTITIONED_TABLE only if we
publish_via_root (aka pub_partopt is PUBLICATION_PART_ROOT). This is
what we are doing in GetAllTablesPublicationRelations. Is there a
reason to be different here?

3.
@@ -538,7 +788,7 @@ RemovePublicationById(Oid pubid)
  if (!HeapTupleIsValid(tup))
  elog(ERROR, "cache lookup failed for publication %u", pubid);

- pubform = (Form_pg_publication)GETSTRUCT(tup);
+ pubform = (Form_pg_publication) GETSTRUCT(tup);

We don't need the above change for this patch. I think this may be due
pgindent but we can do this separately rather than as part of this
patch.

-- 
With Regards,
Amit Kapila.


v35-0001-Added-schema-level-support-for-publication.patch
Description: Binary data


v1-0001-Changes-by-Amit.patch
Description: Binary data

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 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 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

Re: Added schema level support for publication.

[Amit Kapila]

On Wed, Sep 29, 2021 at 5:48 PM houzj.f...@fujitsu.com
 wrote:
>
> On Wed, Sep 29, 2021 5:14 PM Amit Kapila  wrote:
> > On Wed, Sep 29, 2021 at 11:59 AM houzj.f...@fujitsu.com
> >  wrote:
> > >
> > > On Wed, Sep 29, 2021 12:34 PM Amit Kapila 
> > > > 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.
> > > > > 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, ...
> > > > >
> > > >
> > > > Won't this will allow changing one of the partitions for which only
> > > > partitioned table is part of the target schema?
> > >
> > > I think it still disallow changing partition's schema to the published 
> > > one.
> > > I tested with the following SQLs.
> > > -
> > > create schema sch1;
> > > create schema sch2;
> > > create schema sch3;
> > >
> > > create table sch1.tbl1 (a int) partition by range ( a ); create table
> > > sch2.tbl1_part1 partition of sch1.tbl1 for values from (1) to (101);
> > > create table sch3.tbl1_part2 partition of sch1.tbl1 for values from
> > > (101) to (200); create publication pub for ALL TABLES IN schema sch1,
> > > TABLE sch2.tbl1_part1; alter table sch2.tbl1_part1 set schema sch1;
> > > ---* It will report an error here *
> > > -
> > >
> >
> > Use all steps before "create publication" and then try below. These will 
> > give an
> > error with the patch proposed but if I change it to what you are proposing 
> > then
> > it won't give an error.
> > create publication pub for ALL TABLES IN schema sch2, Table sch1.tbl1; alter
> > table sch3.tbl1_part2 set schema sch2;
> >
> > But now again thinking about it, I am not sure if we really want to give 
> > error in
> > this case. What do you think?
>
> Personally, I think we can allow the above case.
>
> Because if user specify the partitioned table in the publication like above,
> they cannot drop the partition separately. And the partitioned table is the
> actual one in pg_publication_rel. So, I think allowing this case seems won't
> make people feel confused.
>

Yeah, I also thought on similar lines. So, let's allow this case.

>
> > Also, if we use list_intersection trick, then how will
> > we tell the publication due to which this problem has occurred, or do you 
> > think
> > we should leave that as an exercise for the user?
>
> I thought list_intersection will return a puboids list in which the puboid 
> exists in both input list.
> We can choose the first puboid and output it in the error message which seems 
> the same as
> the current patch.
>
> But I noticed list_intersection doesn't support T_OidList, so we might need 
> to search the puboid
> Manually. Like:
>
> foreach(cell, relPubids)
> {
> if (list_member_oid(schemaPubids, lfirst_oid(cell)))
> 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(lfirst_oid(cell), false)));
> }
>

Looks good to me.


-- 
With Regards,
Amit Kapila.

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Wed, Sep 29, 2021 5:14 PM Amit Kapila  wrote:
> On Wed, Sep 29, 2021 at 11:59 AM houzj.f...@fujitsu.com
>  wrote:
> >
> > On Wed, Sep 29, 2021 12:34 PM Amit Kapila 
> > > 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.
> > > > 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, ...
> > > >
> > >
> > > Won't this will allow changing one of the partitions for which only
> > > partitioned table is part of the target schema?
> >
> > I think it still disallow changing partition's schema to the published one.
> > I tested with the following SQLs.
> > -
> > create schema sch1;
> > create schema sch2;
> > create schema sch3;
> >
> > create table sch1.tbl1 (a int) partition by range ( a ); create table
> > sch2.tbl1_part1 partition of sch1.tbl1 for values from (1) to (101);
> > create table sch3.tbl1_part2 partition of sch1.tbl1 for values from
> > (101) to (200); create publication pub for ALL TABLES IN schema sch1,
> > TABLE sch2.tbl1_part1; alter table sch2.tbl1_part1 set schema sch1;
> > ---* It will report an error here *
> > -
> >
> 
> Use all steps before "create publication" and then try below. These will give 
> an
> error with the patch proposed but if I change it to what you are proposing 
> then
> it won't give an error.
> create publication pub for ALL TABLES IN schema sch2, Table sch1.tbl1; alter
> table sch3.tbl1_part2 set schema sch2;
> 
> But now again thinking about it, I am not sure if we really want to give 
> error in
> this case. What do you think?

Personally, I think we can allow the above case.

Because if user specify the partitioned table in the publication like above,
they cannot drop the partition separately. And the partitioned table is the
actual one in pg_publication_rel. So, I think allowing this case seems won't
make people feel confused.

Besides, in the current patch, we have allowed similar case in CREATE/ALTER
PUBLICATION cases. In this SQL: "create publication pub for ALL TABLES IN
schema sch2, Table sch1.tbl1;", one of the partitions ' sch2.tbl1_part1' is
from schema 'sch2' which is published. It might be better to make the behavior
consistent.

> Also, if we use list_intersection trick, then how will
> we tell the publication due to which this problem has occurred, or do you 
> think
> we should leave that as an exercise for the user?

I thought list_intersection will return a puboids list in which the puboid 
exists in both input list.
We can choose the first puboid and output it in the error message which seems 
the same as
the current patch.

But I noticed list_intersection doesn't support T_OidList, so we might need to 
search the puboid
Manually. Like:

foreach(cell, relPubids)
{
if (list_member_oid(schemaPubids, lfirst_oid(cell)))
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(lfirst_oid(cell), false)));
}

Best regards,
Hou zj

Re: Added schema level support for publication.

[Amit Kapila]

On Wed, Sep 29, 2021 at 11:59 AM houzj.f...@fujitsu.com
 wrote:
>
> On Wed, Sep 29, 2021 12:34 PM Amit Kapila 
> > 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.
> > >
> > > 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, ...
> > >
> >
> > Won't this will allow changing one of the partitions for which only 
> > partitioned
> > table is part of the target schema?
>
> I think it still disallow changing partition's schema to the published one.
> I tested with the following SQLs.
> -
> create schema sch1;
> create schema sch2;
> create schema sch3;
>
> create table sch1.tbl1 (a int) partition by range ( a );
> create table sch2.tbl1_part1 partition of sch1.tbl1 for values from (1) to 
> (101);
> create table sch3.tbl1_part2 partition of sch1.tbl1 for values from (101) to 
> (200);
> create publication pub for ALL TABLES IN schema sch1, TABLE sch2.tbl1_part1;
> alter table sch2.tbl1_part1 set schema sch1;
> ---* It will report an error here *
> -
>

Use all steps before "create publication" and then try below. These
will give an error with the patch proposed but if I change it to what
you are proposing then it won't give an error.
create publication pub for ALL TABLES IN schema sch2, Table sch1.tbl1;
alter table sch3.tbl1_part2 set schema sch2;

But now again thinking about it, I am not sure if we really want to
give error in this case. What do you think? Also, if we use
list_intersection trick, then how will we tell the publication due to
which this problem has occurred, or do you think we should leave that
as an exercise for the user?

-- 
With Regards,
Amit Kapila.

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

On Wed, Sep 29, 2021 12:34 PM Amit Kapila 
> 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.
> >
> > 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, ...
> >
> 
> Won't this will allow changing one of the partitions for which only 
> partitioned
> table is part of the target schema?

I think it still disallow changing partition's schema to the published one.
I tested with the following SQLs.
-
create schema sch1;
create schema sch2;
create schema sch3;

create table sch1.tbl1 (a int) partition by range ( a );
create table sch2.tbl1_part1 partition of sch1.tbl1 for values from (1) to 
(101);
create table sch3.tbl1_part2 partition of sch1.tbl1 for values from (101) to 
(200);
create publication pub for ALL TABLES IN schema sch1, TABLE sch2.tbl1_part1;
alter table sch2.tbl1_part1 set schema sch1;
---* It will report an error here *
-

Did I miss something ?

Best regards,
Hou zj

Re: Added schema level support for publication.

[Greg Nancarrow]

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."

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Added schema level support for publication.

[Amit Kapila]

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."

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

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

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.

5.
ObjectsInPublicationToOids()
{
..
+ pubobj = (PublicationObjSpec *) lfirst(cell);
+ if (pubobj->pubobjtype == PUBLICATIONOBJ_TABLE)

It is better to keep an empty line between above two lines.

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.

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."

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.

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."

-- 
With Regards,
Amit Kapila.

Re: Added schema level support for publication.

[Amit Kapila]

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.
>
> 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, ...
>

Won't this will allow changing one of the partitions for which only
partitioned table is part of the target schema? And then probably we
won't be able to provide the exact publication in the error message if
we followed the above?

-- 
With Regards,
Amit Kapila.

RE: Added schema level support for publication.

[houzj.f...@fujitsu.com]

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 [[, ...]

[[ -> [

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 */

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, ...

Best regards,
Hou zj

RE: Added schema level support for publication.

[tanghy.f...@fujitsu.com]

On Monday, Tuesday, September 28, 2021 10:49 PM, vignesh C 
 wrote:
> 
> 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.g
> mail.com
> 

Thanks for fixing it. I confirmed the error can be output as expected.

Here is a problem related to publish_via_partition_root option when using this
patch. With this option on, I think pg_get_publication_tables function gave an
unexcepted result and the subscriber would get dual data during table sync.


For example:
(I used pg_publication_tables view to make it looks clearer)

create schema sch1;
create table sch1.tbl1 (a int) partition by range ( a );
create table sch1.tbl1_part1 partition of sch1.tbl1 for values from (1) to (10);
create table sch1.tbl1_part2 partition of sch1.tbl1 for values from (10) to 
(20);
create table sch1.tbl1_part3 partition of sch1.tbl1 for values from (20) to 
(30);
create publication pub for all tables in schema sch1 
with(publish_via_partition_root=1);

postgres=# select * from pg_publication_tables where pubname='pub';
 pubname | schemaname | tablename
-++
 pub | sch1   | tbl1_part1
 pub | sch1   | tbl1_part2
 pub | sch1   | tbl1_part3
 pub | sch1   | tbl1
(4 rows)


It shows both the partitioned table and its leaf partitions. But the result of
FOR ALL TABLES publication couldn't show the leaf partitions.


postgres=# create publication pub_all for all tables 
with(publish_via_partition_root=1);
CREATE PUBLICATION
postgres=# select * from pg_publication_tables where pubname='pub_all';
 pubname | schemaname | tablename
-++---
 pub_all | sch1   | tbl1
(1 row)


How about make the following change to avoid it? I tried it and it also fixed 
dual
data issue during table sync.


diff --git a/src/backend/catalog/pg_publication.c 
b/src/backend/catalog/pg_publication.c
index 04e785b192..4e8ccdabc6 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -632,7 +632,8 @@ GetSchemaPublicationRelations(Oid schemaid, 
PublicationPartOpt pub_partopt)
Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
Oid relid = relForm->oid;

-   if (is_publishable_class(relid, relForm))
+   if (is_publishable_class(relid, relForm) &&
+   !(relForm->relispartition && pub_partopt == 
PUBLICATION_PART_ROOT))
result = lappend_oid(result, relid);
}


Regards
Tang

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.

[tanghy.f...@fujitsu.com]

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?

Regards
Tang

Re: Added schema level support for publication.

[Greg Nancarrow]

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

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.

Regards,
Greg Nancarrow
Fujitsu Australia