Message-ID: <2ed1e3ff-076d-1127-555a-38b5d6c135ba@enterprisedb.com>
Date: Wed, 15 Dec 2021 17:56:04 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: logical decoding and replication of sequences
Content-Language: en-US
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Petr Jelinek <petr.jelinek@enterprisedb.com>
References: <202112151642.h7cehn6zplxg@alvherre.pgsql>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <202112151642.h7cehn6zplxg@alvherre.pgsql>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/2ed1e3ff-076d-1127-555a-38b5d6c135ba%40enterprisedb.com>
Precedence: bulk


On 12/15/21 17:42, Alvaro Herrera wrote:
> 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".
> 

Thanks for the review. I'm aware 0003 is still incomplete and subject to
change - it's certainly not meant for commit yet. The current 0003 patch
is sufficient for testing the infrastructure, but we need to figure out
how to make it easier to use, what to do with implicit sequences and
similar things. Peter had some ideas in [1].

[1]
https://www.postgresql.org/message-id/359bf6d0-413d-292a-4305-e99eeafead39%40enterprisedb.com

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