Date: Wed, 15 Dec 2021 13:42:55 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Petr Jelinek <petr.jelinek@enterprisedb.com>
Subject: Re: logical decoding and replication of sequences
Message-ID: <202112151642.h7cehn6zplxg@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ae3cab67-c31e-b527-dd73-08f196999ad4@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202112151642.h7cehn6zplxg%40alvherre.pgsql>
Precedence: bulk

Looking at 0003,

On 2021-Dec-14, Tomas Vondra wrote:

> diff --git a/doc/src/sgml/ref/alter_publication.sgml b/doc/src/sgml/ref/alter_publication.sgml
> index bb4ef5e5e22..4d166ad3f9c 100644
> --- a/doc/src/sgml/ref/alter_publication.sgml
> +++ b/doc/src/sgml/ref/alter_publication.sgml
> @@ -31,7 +31,9 @@ ALTER PUBLICATION <replaceable class="parameter">name</replaceable> RENAME TO <r
>  <phrase>where <replaceable class="parameter">publication_object</replaceable> is one of:</phrase>
>  
>      TABLE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [, ... ]
> +    SEQUENCE <replaceable class="parameter">sequence_name</replaceable> [ * ] [, ... ]
>      ALL TABLES IN SCHEMA { <replaceable class="parameter">schema_name</replaceable> | CURRENT_SCHEMA } [, ... ]
> +    ALL SEQUENCE IN SCHEMA { <replaceable class="parameter">schema_name</replaceable> | CURRENT_SCHEMA } [, ... ]

Note that this says ALL SEQUENCE; I think it should be ALL SEQUENCES.

> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 3d4dd43e47b..f037c17985b 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -9762,6 +9762,26 @@ PublicationObjSpec:
...
> +			| ALL SEQUENCE IN_P SCHEMA ColId
> +				{
> +					$$ = makeNode(PublicationObjSpec);
> +					$$->pubobjtype = PUBLICATIONOBJ_SEQUENCE_IN_SCHEMA;
> +					$$->name = $5;
> +					$$->location = @5;
> +				}
> +			| ALL SEQUENCES IN_P SCHEMA CURRENT_SCHEMA
> +				{
> +					$$ = makeNode(PublicationObjSpec);
> +					$$->pubobjtype = PUBLICATIONOBJ_SEQUENCE_IN_CUR_SCHEMA;
> +					$$->location = @5;
> +				}

And here you have ALL SEQUENCE in one spot and ALL SEQUENCES in the
other.

BTW I think these enum values should use the plural too,
PUBLICATIONOBJ_SEQUENCES_IN_CUR_SCHEMA (not SEQUENCE).  I suppose you
copied from PUBLICATIONOBJ_TABLE_IN_CUR_SCHEMA, but that too seems to be
a mistake: should be PUBLICATIONOBJ_TABLES_IN_CUR_SCHEMA.


> @@ -10097,6 +10117,12 @@ UnlistenStmt:
>  				}
>  		;
>  
> +/*
> + * FIXME
> + *
> + * opt_publication_for_sequences and publication_for_sequences should be
> + * copies for sequences
> + */

Not sure if this FIXME is relevant or should just be removed.

> @@ -10105,6 +10131,12 @@ UnlistenStmt:
>   *		BEGIN / COMMIT / ROLLBACK
>   *		(also older versions END / ABORT)
>   *
> + * ALTER PUBLICATION name ADD SEQUENCE sequence [, sequence2]
> + *
> + * ALTER PUBLICATION name DROP SEQUENCE sequence [, sequence2]
> + *
> + * ALTER PUBLICATION name SET SEQUENCE sequence [, sequence2]
> + *

This comment addition seems misplaced?

> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index 2f412ca3db3..e30bf7b1b55 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -1647,13 +1647,13 @@ psql_completion(const char *text, int start, int end)
>  		COMPLETE_WITH("ADD", "DROP", "OWNER TO", "RENAME TO", "SET");
>  	/* ALTER PUBLICATION <name> ADD */
>  	else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD"))
> -		COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE");
> +		COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE|SEQUENCE");
>  	/* ALTER PUBLICATION <name> DROP */
>  	else if (Matches("ALTER", "PUBLICATION", MatchAny, "DROP"))
> -		COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE");
> +		COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE|SEQUENCE");
>  	/* ALTER PUBLICATION <name> SET */
>  	else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET"))
> -		COMPLETE_WITH("(", "ALL TABLES IN SCHEMA", "TABLE");
> +		COMPLETE_WITH("(", "ALL TABLES IN SCHEMA", "TABLE|SEQUENCE");

I think you should also add "ALL SEQUENCES IN SCHEMA" to these lists.


>  	else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD|DROP|SET", "ALL", "TABLES", "IN", "SCHEMA"))

... and perhaps make this "ALL", "TABLES|SEQUENCES", "IN", "SCHEMA".


-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)