public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v7 1/4] Introduce a new syntax to add/drop publications
8+ messages / 6 participants
[nested] [flat]
* [PATCH v7 1/4] Introduce a new syntax to add/drop publications
@ 2021-03-07 12:56 Japin Li <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Japin Li @ 2021-03-07 12:56 UTC (permalink / raw)
At present, if we want to update publications in subscription, we can
use SET PUBLICATION, however, it requires supply all publications that
exists and the new publications if we want to add new publications, it's
inconvenient. The new syntax only supply the new publications. When
the refresh is true, it only refresh the new publications.
---
src/backend/commands/subscriptioncmds.c | 132 ++++++++++++++++++++++++
src/backend/parser/gram.y | 20 ++++
src/include/nodes/parsenodes.h | 2 +
3 files changed, 154 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bfd3514546..bf1e579e6f 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -47,6 +47,7 @@
#include "utils/syscache.h"
static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
+static List *merge_publications(List *oldpublist, List *newpublist, bool addpub);
static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err);
@@ -964,6 +965,53 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
break;
}
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ {
+ bool copy_data = false;
+ bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
+ bool refresh;
+ List *publist = NIL;
+
+ publist = merge_publications(sub->publications, stmt->publication, isadd);
+
+ parse_subscription_options(stmt->options,
+ NULL, /* no "connect" */
+ NULL, NULL, /* no "enabled" */
+ NULL, /* no "create_slot" */
+ NULL, NULL, /* no "slot_name" */
+ isadd ? ©_data : NULL, /* for drop, no "copy_data" */
+ NULL, /* no "synchronous_commit" */
+ &refresh,
+ NULL, NULL, /* no "binary" */
+ NULL, NULL); /* no "streaming" */
+
+ values[Anum_pg_subscription_subpublications - 1] =
+ publicationListToArray(publist);
+ replaces[Anum_pg_subscription_subpublications - 1] = true;
+
+ update_tuple = true;
+
+ /* Refresh if user asked us to. */
+ if (refresh)
+ {
+ if (!sub->enabled)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"),
+ errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).")));
+
+ PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
+
+ /* Only refresh the added/dropped list of publications. */
+ sub->publications = stmt->publication;
+
+ AlterSubscription_refresh(sub, copy_data);
+ }
+
+ break;
+ }
+
case ALTER_SUBSCRIPTION_REFRESH:
{
bool copy_data;
@@ -1551,3 +1599,87 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
errhint("Use %s to disassociate the subscription from the slot.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
}
+
+/*
+ * Merge current subscription's publications and user specified publications
+ * by ADD/DROP PUBLICATIONS.
+ *
+ * If addpub is true, we will add the list of publications into oldpublist.
+ * Otherwise, we will delete the list of publications from oldpublist.
+ */
+static List *
+merge_publications(List *oldpublist, List *newpublist, bool addpub)
+{
+ StringInfoData errstr;
+ int errstrcnt = 0;
+ ListCell *lc;
+
+ initStringInfo(&errstr);
+
+ foreach(lc, newpublist)
+ {
+ char *name = strVal(lfirst(lc));
+ ListCell *cell = NULL;
+
+ foreach(cell, oldpublist)
+ {
+ char *pubname = strVal(lfirst(cell));
+
+ if (strcmp(name, pubname) == 0)
+ {
+ if (addpub)
+ {
+ errstrcnt++;
+
+ if (errstrcnt == 1)
+ appendStringInfo(&errstr, _("\"%s\""), name);
+ else
+ appendStringInfo(&errstr, _(", \"%s\""), name);
+ }
+ else
+ oldpublist = list_delete_cell(oldpublist, cell);
+
+ break;
+ }
+ }
+
+ if (addpub && cell == NULL)
+ oldpublist = lappend(oldpublist, makeString(name));
+ else if (!addpub && cell == NULL)
+ {
+ errstrcnt++;
+
+ if (errstrcnt == 1)
+ appendStringInfo(&errstr, _("\"%s\""), name);
+ else
+ appendStringInfo(&errstr, _(", \"%s\""), name);
+ }
+ }
+
+ if (errstrcnt >= 1)
+ {
+ if (addpub)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg_plural("publication %s is already present in the subscription",
+ "publications %s are already present in the subscription",
+ errstrcnt, errstr.data)));
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg_plural("publication %s doesn't exist in the subscription",
+ "publications %s do not exist in the subscription",
+ errstrcnt, errstr.data)));
+ }
+ }
+
+ if (oldpublist == NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("subscription must contain at least one publication")));
+
+ return oldpublist;
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 652be0b96d..6d75e82096 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9609,6 +9609,26 @@ AlterSubscriptionStmt:
n->options = $6;
$$ = (Node *)n;
}
+ | ALTER SUBSCRIPTION name ADD_P PUBLICATION name_list opt_definition
+ {
+ AlterSubscriptionStmt *n =
+ makeNode(AlterSubscriptionStmt);
+ n->kind = ALTER_SUBSCRIPTION_ADD_PUBLICATION;
+ n->subname = $3;
+ n->publication = $6;
+ n->options = $7;
+ $$ = (Node *)n;
+ }
+ | ALTER SUBSCRIPTION name DROP PUBLICATION name_list opt_definition
+ {
+ AlterSubscriptionStmt *n =
+ makeNode(AlterSubscriptionStmt);
+ n->kind = ALTER_SUBSCRIPTION_DROP_PUBLICATION;
+ n->subname = $3;
+ n->publication = $6;
+ n->options = $7;
+ $$ = (Node *)n;
+ }
| ALTER SUBSCRIPTION name SET PUBLICATION name_list opt_definition
{
AlterSubscriptionStmt *n =
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 236832a2ca..e109607936 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3580,6 +3580,8 @@ typedef enum AlterSubscriptionType
ALTER_SUBSCRIPTION_OPTIONS,
ALTER_SUBSCRIPTION_CONNECTION,
ALTER_SUBSCRIPTION_PUBLICATION,
+ ALTER_SUBSCRIPTION_ADD_PUBLICATION,
+ ALTER_SUBSCRIPTION_DROP_PUBLICATION,
ALTER_SUBSCRIPTION_REFRESH,
ALTER_SUBSCRIPTION_ENABLED
} AlterSubscriptionType;
--
2.25.1
--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment;
filename=v7-0002-Test-ALTER-SUBSCRIPTION-.-ADD-DROP-PUBLICATION.patch
^ permalink raw reply [nested|flat] 8+ messages in thread
* GUC names in messages
@ 2023-11-01 09:02 Peter Smith <[email protected]>
2023-11-01 09:22 ` Re: GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:23 ` Re: GUC names in messages Daniel Gustafsson <[email protected]>
0 siblings, 2 replies; 8+ messages in thread
From: Peter Smith @ 2023-11-01 09:02 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Hi hackers,
While reviewing another patch I noticed how the GUCs are
inconsistently named within the GUC_check_errdetail messages:
======
below, the GUC name is embedded but not quoted:
src/backend/access/transam/xlogprefetcher.c:
GUC_check_errdetail("recovery_prefetch is not supported on platforms
that lack posix_fadvise().");
src/backend/access/transam/xlogrecovery.c:
GUC_check_errdetail("recovery_target_timeline is not a valid
number.");
src/backend/commands/variable.c:
GUC_check_errdetail("effective_io_concurrency must be set to 0 on
platforms that lack posix_fadvise().");
src/backend/commands/variable.c:
GUC_check_errdetail("maintenance_io_concurrency must be set to 0 on
platforms that lack posix_fadvise().");
src/backend/port/sysv_shmem.c:
GUC_check_errdetail("huge_page_size must be 0 on this platform.");
src/backend/port/win32_shmem.c:
GUC_check_errdetail("huge_page_size must be 0 on this platform.");
src/backend/replication/syncrep.c:
GUC_check_errdetail("synchronous_standby_names parser failed");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported on this
platform.");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported for WAL because
XLOG_BLCKSZ is too small");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported for data because
BLCKSZ is too small");
src/backend/tcop/postgres.c:
GUC_check_errdetail("client_connection_check_interval must be set to 0
on this platform.");
~~~
below, the GUC name is embedded and double-quoted:
src/backend/commands/vacuum.c:
GUC_check_errdetail("\"vacuum_buffer_usage_limit\" must be 0 or
between %d kB and %d kB",
src/backend/commands/variable.c:
GUC_check_errdetail("Conflicting \"datestyle\" specifications.");
src/backend/storage/buffer/localbuf.c:
GUC_check_errdetail("\"temp_buffers\" cannot be changed after any
temporary tables have been accessed in the session.");
src/backend/tcop/postgres.c:
GUC_check_errdetail("\"max_stack_depth\" must not exceed %ldkB.",
src/backend/tcop/postgres.c: GUC_check_errdetail("Cannot enable
parameter when \"log_statement_stats\" is true.");
src/backend/tcop/postgres.c: GUC_check_errdetail("Cannot enable
\"log_statement_stats\" when "
~~~
below, the GUC name is substituted but not quoted:
src/backend/access/table/tableamapi.c: GUC_check_errdetail("%s
cannot be empty.",
src/backend/access/table/tableamapi.c: GUC_check_errdetail("%s is
too long (maximum %d characters).",
~~~
I had intended to make a patch to address the inconsistency, but
couldn't decide which of those styles was the preferred one.
Then I worried this could be the tip of the iceberg -- GUC names occur
in many other error messages where they are sometimes quoted and
sometimes not quoted:
e.g. Not quoted -- errhint("You might need to run fewer transactions
at a time or increase max_connections.")));
e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
\"wal_segment_size\"")));
Ideally, they should all look the same everywhere, shouldn't they?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: GUC names in messages
2023-11-01 09:02 GUC names in messages Peter Smith <[email protected]>
@ 2023-11-01 09:22 ` Peter Smith <[email protected]>
2023-11-01 09:59 ` Re: GUC names in messages Daniel Gustafsson <[email protected]>
1 sibling, 1 reply; 8+ messages in thread
From: Peter Smith @ 2023-11-01 09:22 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
On Wed, Nov 1, 2023 at 8:02 PM Peter Smith <[email protected]> wrote:
...
>
> I had intended to make a patch to address the inconsistency, but
> couldn't decide which of those styles was the preferred one.
>
> Then I worried this could be the tip of the iceberg -- GUC names occur
> in many other error messages where they are sometimes quoted and
> sometimes not quoted:
> e.g. Not quoted -- errhint("You might need to run fewer transactions
> at a time or increase max_connections.")));
> e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
> \"wal_segment_size\"")));
>
> Ideally, they should all look the same everywhere, shouldn't they?
>
One idea to achieve consistency might be to always substitute GUC
names using a macro.
#define GUC_NAME(s) ("\"" s "\"")
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s must be at least twice %s",
GUC_NAME("max_wal_size"),
GUC_NAME("wal_segment_size"))));
Thoughts?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: GUC names in messages
2023-11-01 09:02 GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:22 ` Re: GUC names in messages Peter Smith <[email protected]>
@ 2023-11-01 09:59 ` Daniel Gustafsson <[email protected]>
2023-11-01 14:25 ` Re: GUC names in messages Tom Lane <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Gustafsson @ 2023-11-01 09:59 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> On 1 Nov 2023, at 10:22, Peter Smith <[email protected]> wrote:
>
> On Wed, Nov 1, 2023 at 8:02 PM Peter Smith <[email protected]> wrote:
> ...
>>
>> I had intended to make a patch to address the inconsistency, but
>> couldn't decide which of those styles was the preferred one.
>>
>> Then I worried this could be the tip of the iceberg -- GUC names occur
>> in many other error messages where they are sometimes quoted and
>> sometimes not quoted:
>> e.g. Not quoted -- errhint("You might need to run fewer transactions
>> at a time or increase max_connections.")));
>> e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
>> \"wal_segment_size\"")));
>>
>> Ideally, they should all look the same everywhere, shouldn't they?
>>
>
> One idea to achieve consistency might be to always substitute GUC
> names using a macro.
>
> #define GUC_NAME(s) ("\"" s "\"")
>
> ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("%s must be at least twice %s",
> GUC_NAME("max_wal_size"),
> GUC_NAME("wal_segment_size"))));
Something like this might make translations harder since the remaining string
leaves little context about the message. We already have that today to some
extent (so it might not be an issue), and it might be doable to automatically
add translator comments, but it's something to consider.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: GUC names in messages
2023-11-01 09:02 GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:22 ` Re: GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:59 ` Re: GUC names in messages Daniel Gustafsson <[email protected]>
@ 2023-11-01 14:25 ` Tom Lane <[email protected]>
2023-11-01 20:12 ` Re: GUC names in messages Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2023-11-01 14:25 UTC (permalink / raw)
To: Daniel Gustafsson <[email protected]>; +Cc: Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]>
Daniel Gustafsson <[email protected]> writes:
> On 1 Nov 2023, at 10:22, Peter Smith <[email protected]> wrote:
>> One idea to achieve consistency might be to always substitute GUC
>> names using a macro.
>>
>> #define GUC_NAME(s) ("\"" s "\"")
>>
>> ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> errmsg("%s must be at least twice %s",
>> GUC_NAME("max_wal_size"),
>> GUC_NAME("wal_segment_size"))));
> Something like this might make translations harder since the remaining string
> leaves little context about the message. We already have that today to some
> extent (so it might not be an issue), and it might be doable to automatically
> add translator comments, but it's something to consider.
Our error message style guidelines say not to assemble messages out
of separate parts, because it makes translation difficult. Originally
we applied that rule to GUC names mentioned in messages as well.
Awhile ago the translation team decided that that made for too many
duplicative translations, so they'd be willing to compromise on
substituting GUC names. That's only been changed in a haphazard
fashion though, mostly in cases where there actually were duplicative
messages that could be merged this way. And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s. So that's why we have
a mishmash right now.
I'm not enamored of the GUC_NAME idea suggested above. I don't
think it buys anything, and what it does do is make *every single
one* of our GUC-mentioning messages wrong. I think if we want to
standardize here, we should standardize on something that's
already pretty common in the code base.
Another problem with the idea as depicted above is that it
mistakenly assumes that "..." is the correct quoting method
in all languages. You could make GUC_NAME be a pure no-op
macro and continue to put quoting in the translatable string
where it belongs, but then the macro brings even less value.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: GUC names in messages
2023-11-01 09:02 GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:22 ` Re: GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:59 ` Re: GUC names in messages Daniel Gustafsson <[email protected]>
2023-11-01 14:25 ` Re: GUC names in messages Tom Lane <[email protected]>
@ 2023-11-01 20:12 ` Peter Eisentraut <[email protected]>
2023-11-01 20:46 ` Re: GUC names in messages Laurenz Albe <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2023-11-01 20:12 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Daniel Gustafsson <[email protected]>; +Cc: Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]>
On 01.11.23 10:25, Tom Lane wrote:
> And there's never been any
> real clarity about whether to quote GUC names, though certainly we're
> more likely to quote anything injected with %s. So that's why we have
> a mishmash right now.
I'm leaning toward not quoting GUC names. The quoting is needed in
places where the value can be arbitrary, to avoid potential confusion.
But the GUC names are well-known, and we wouldn't add confusing GUC
names like "table" or "not found" in the future.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: GUC names in messages
2023-11-01 09:02 GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:22 ` Re: GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:59 ` Re: GUC names in messages Daniel Gustafsson <[email protected]>
2023-11-01 14:25 ` Re: GUC names in messages Tom Lane <[email protected]>
2023-11-01 20:12 ` Re: GUC names in messages Peter Eisentraut <[email protected]>
@ 2023-11-01 20:46 ` Laurenz Albe <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Laurenz Albe @ 2023-11-01 20:46 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; Daniel Gustafsson <[email protected]>; +Cc: Peter Smith <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, 2023-11-01 at 16:12 -0400, Peter Eisentraut wrote:
> On 01.11.23 10:25, Tom Lane wrote:
> > And there's never been any
> > real clarity about whether to quote GUC names, though certainly we're
> > more likely to quote anything injected with %s. So that's why we have
> > a mishmash right now.
>
> I'm leaning toward not quoting GUC names. The quoting is needed in
> places where the value can be arbitrary, to avoid potential confusion.
> But the GUC names are well-known, and we wouldn't add confusing GUC
> names like "table" or "not found" in the future.
I agree for names with underscores in them. But I think that quoting
is necessary for names like "timezone" or "datestyle" that might be
mistaken for normal words. My personal preference is to always quote
GUC names, but I think it is OK not to quote GOCs whose name are
clearly not natural language words.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: GUC names in messages
2023-11-01 09:02 GUC names in messages Peter Smith <[email protected]>
@ 2023-11-01 09:23 ` Daniel Gustafsson <[email protected]>
1 sibling, 0 replies; 8+ messages in thread
From: Daniel Gustafsson @ 2023-11-01 09:23 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> On 1 Nov 2023, at 10:02, Peter Smith <[email protected]> wrote:
> GUC_check_errdetail("effective_io_concurrency must be set to 0 on
> platforms that lack posix_fadvise().");
> src/backend/commands/variable.c:
> GUC_check_errdetail("maintenance_io_concurrency must be set to 0 on
> platforms that lack posix_fadvise().");
These should be substituted to reduce the number of distinct messages that need
to be translated. I wouldn't be surprised if more like these have slipped
through.
> I had intended to make a patch to address the inconsistency, but
> couldn't decide which of those styles was the preferred one.
Given the variety in the codebase I don't think there is a preferred one.
> Then I worried this could be the tip of the iceberg
All good rabbit-holes uncovered during hacking are.. =)
> Ideally, they should all look the same everywhere, shouldn't they?
Having a policy would be good, having one which is known and enforced is even
better (like how we are consistent around error messages based on our Error
Message Style Guide).
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-11-01 20:46 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-07 12:56 [PATCH v7 1/4] Introduce a new syntax to add/drop publications Japin Li <[email protected]>
2023-11-01 09:02 GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:22 ` Re: GUC names in messages Peter Smith <[email protected]>
2023-11-01 09:59 ` Re: GUC names in messages Daniel Gustafsson <[email protected]>
2023-11-01 14:25 ` Re: GUC names in messages Tom Lane <[email protected]>
2023-11-01 20:12 ` Re: GUC names in messages Peter Eisentraut <[email protected]>
2023-11-01 20:46 ` Re: GUC names in messages Laurenz Albe <[email protected]>
2023-11-01 09:23 ` Re: GUC names in messages Daniel Gustafsson <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox