Thread: pub/sub - specifying optional parameters without values.
Hi hackers. This post is about parameter default values. Specifically. it's about the CREATE PUBLICATION and CREATE SUBSCRIPTION syntax, although the same issue might apply to other commands I am unaware of... ~~~ Background: CREATE PUBLICATION syntax has a WITH clause: [ WITH ( publication_parameter [= value] [, ... ] ) ] CREATE SUBSCRIPTION syntax has a similar clause: [ WITH ( subscription_parameter [= value] [, ... ] ) ] ~~~ The docs describe all the parameters that can be specified. Parameters are optional, so the docs describe the defaults if the parameter name is not specified. However, notice that the parameter *value* part is also optional. So, what is the defined behaviour if a parameter name is specified but no *value* is given? In practice, it seems to just be a shorthand for assigning a boolean value to true... BUT - a) I can't find anywhere in the docs where it actually says this b) Without documentation some might consider it to be strange that now there are 2 kinds of defaults - a default when there is no name, and another default when there is no value - and those are not always the same. e.g. if publish_via_partition root is not specified at all, it is equivalent of WITH (publish_via_partition_root=false), but OTOH, WITH (publish_via_partition_root) is equivalent of WITH (publish_via_partition_root=true). c) What about non-boolean parameters? In practice, it seems they all give errors: test_pub=# CREATE PUBLICATION pub99 FOR ALL TABLES WITH (publish); ERROR: publish requires a parameter test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'host=localhost dbname=test_pub' PUBLICATION pub1 WITH (slot_name); ERROR: slot_name requires a parameter test_sub=# CREATE SUBSCRIPTION sub1 CONNECTION 'host=localhost dbname=test_pub' PUBLICATION pub1 WITH (synchronous_commit); ERROR: synchronous_commit requires a parameter ~~~ It almost feels like this is an undocumented feature, except it isn't quite undocumented because it is right there in black-and-white in the syntax "[= value]". Or perhaps this implied boolean-true behaviour is already described elsewhere? But if it is, I have not found it yet. IMO a simple patch (PSA) is needed to clarify the behaviour. Thoughts? ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Fri, Oct 14, 2022 at 07:54:37PM +1100, Peter Smith wrote: > Hi hackers. > > This post is about parameter default values. Specifically. it's about > the CREATE PUBLICATION and CREATE SUBSCRIPTION syntax, although the > same issue might apply to other commands I am unaware of... The same thing seems to be true in various other pages: git grep 'WITH.*value' doc In addition to WITH, it's also true of SET: git grep -F '[= <replaceable class="parameter">value' doc/src/sgml/ref/alter_index.sgml doc/src/sgml/ref/alter_table.sgmldoc/src/sgml/ref/create_materialized_view.sgml doc/src/sgml/ref/create_publication.sgmldoc/src/sgml/ref/create_subscription.sgml Note that some utility statements (analyze,cluster,vacuum,reindex) which have parenthesized syntax with booleans say this: | The boolean value can also be omitted, in which case TRUE is assumed. BTW, in your patch: + <para> + A <type>boolean</type> parameter can omit the value. This is equivalent + to assigning the parameter to <literal>true</literal>. + </para> + <para> should say: "The value can be omitted, which is equivalent to specifying TRUE". -- Justin
On Tue, Oct 18, 2022 at 7:09 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Fri, Oct 14, 2022 at 07:54:37PM +1100, Peter Smith wrote: > > Hi hackers. > > > > This post is about parameter default values. Specifically. it's about > > the CREATE PUBLICATION and CREATE SUBSCRIPTION syntax, although the > > same issue might apply to other commands I am unaware of... > > The same thing seems to be true in various other pages: > git grep 'WITH.*value' doc > > In addition to WITH, it's also true of SET: > > git grep -F '[= <replaceable class="parameter">value' doc/src/sgml/ref/alter_index.sgml doc/src/sgml/ref/alter_table.sgmldoc/src/sgml/ref/create_materialized_view.sgml doc/src/sgml/ref/create_publication.sgmldoc/src/sgml/ref/create_subscription.sgml > > Note that some utility statements (analyze,cluster,vacuum,reindex) which > have parenthesized syntax with booleans say this: > | The boolean value can also be omitted, in which case TRUE is assumed. Thank you for the feedback and for reporting about other places similar to this. For now, I only intended to fix docs related to logical replication. Scope creep to other areas maybe can be addressed by subsequent patches if this one gets accepted. > > BTW, in your patch: > + <para> > + A <type>boolean</type> parameter can omit the value. This is equivalent > + to assigning the parameter to <literal>true</literal>. > + </para> > + <para> > > should say: "The value can be omitted, which is equivalent to specifying > TRUE". > I've changed the text as you suggested, except in a couple of places where I qualified by saying "For boolean parameters..."; that's because the value part is not *always* optional. I've also made similar updates to the ALTER PUBLICATION/SUBSCRIPTION pages, which were accidentally missed before. ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
Hi, This documentation change looks good to me. I verified in testing and in code that the value for boolean parameters in PUB/SUBcommands can be omitted. which is equivalent to specifying TRUE. For example, CREATE PUBLICATIOIN mypub for ALL TABLES with (publish_via_partition_root); is equivalent to CREATE PUBLICATIOIN mypub for ALL TABLES with (publish_via_partition_root = TRUE); The behavior is due to the following code https://github.com/postgres/postgres/blob/master/src/backend/commands/define.c#L113 Marking this as ready for committer. The new status of this patch is: Ready for Committer
Zheng Li <zhengli10@gmail.com> writes: > The behavior is due to the following code > https://github.com/postgres/postgres/blob/master/src/backend/commands/define.c#L113 Yeah, so you can grep for places that have this behavior by looking for defGetBoolean calls ... and there are quite a few. That leads me to the conclusion that we'd better invent a fairly stylized documentation solution that we can plug into a lot of places, rather than thinking of slightly different ways to say it and places to say it. I'm not necessarily opposed to Peter's desire to fix replication-related commands first, but we have more to do later. I'm also not that thrilled with putting the addition up at the top of the relevant text. This behavior is at least two decades old, so if we've escaped documenting it at all up to now, it can't be that important to most people. I also notice that ALTER SUBSCRIPTION has fully three different sub-sections with about equal claims on this note, if we're going to stick it directly into the affected option lists. That all leads me to propose that we add the new text at the end of the Parameters <refsect1> in the affected man pages. So about like the attached. (I left out alter_publication.sgml, as I'm not sure it needs its own copy of this text --- it doesn't describe individual parameters at all, just refer to CREATE PUBLICATION.) regards, tom lane diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml index ad93553a1d..964fcbb8ff 100644 --- a/doc/src/sgml/ref/alter_subscription.sgml +++ b/doc/src/sgml/ref/alter_subscription.sgml @@ -277,6 +277,13 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO < </listitem> </varlistentry> </variablelist> + + <para> + When specifying a parameter of type <type>boolean</type>, the + <literal>=</literal> <replaceable class="parameter">value</replaceable> + part can be omitted, which is equivalent to + specifying <literal>TRUE</literal>. + </para> </refsect1> <refsect1> diff --git a/doc/src/sgml/ref/create_publication.sgml b/doc/src/sgml/ref/create_publication.sgml index e229384e6f..370dac2ccf 100644 --- a/doc/src/sgml/ref/create_publication.sgml +++ b/doc/src/sgml/ref/create_publication.sgml @@ -217,6 +217,13 @@ CREATE PUBLICATION <replaceable class="parameter">name</replaceable> </varlistentry> </variablelist> + + <para> + When specifying a parameter of type <type>boolean</type>, the + <literal>=</literal> <replaceable class="parameter">value</replaceable> + part can be omitted, which is equivalent to + specifying <literal>TRUE</literal>. + </para> </refsect1> <refsect1> diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml index eba72c6af6..51c45f17c7 100644 --- a/doc/src/sgml/ref/create_subscription.sgml +++ b/doc/src/sgml/ref/create_subscription.sgml @@ -354,6 +354,13 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl </listitem> </varlistentry> </variablelist> + + <para> + When specifying a parameter of type <type>boolean</type>, the + <literal>=</literal> <replaceable class="parameter">value</replaceable> + part can be omitted, which is equivalent to + specifying <literal>TRUE</literal>. + </para> </refsect1> <refsect1 id="sql-createsubscription-notes" xreflabel="Notes">
On Mon, Jan 30, 2023 at 8:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Zheng Li <zhengli10@gmail.com> writes: > > The behavior is due to the following code > > https://github.com/postgres/postgres/blob/master/src/backend/commands/define.c#L113 > > Yeah, so you can grep for places that have this behavior by looking > for defGetBoolean calls ... and there are quite a few. That leads > me to the conclusion that we'd better invent a fairly stylized > documentation solution that we can plug into a lot of places, > rather than thinking of slightly different ways to say it and > places to say it. I'm not necessarily opposed to Peter's desire > to fix replication-related commands first, but we have more to do > later. > > I'm also not that thrilled with putting the addition up at the top > of the relevant text. This behavior is at least two decades old, > so if we've escaped documenting it at all up to now, it can't be > that important to most people. > > I also notice that ALTER SUBSCRIPTION has fully three different > sub-sections with about equal claims on this note, if we're going > to stick it directly into the affected option lists. > > That all leads me to propose that we add the new text at the end of > the Parameters <refsect1> in the affected man pages. So about > like the attached. (I left out alter_publication.sgml, as I'm not > sure it needs its own copy of this text --- it doesn't describe > individual parameters at all, just refer to CREATE PUBLICATION.) > The v3 patch LGTM (just for the logical replication commands). ------ Kind Regards, Peter Smith. Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
> The v3 patch LGTM (just for the logical replication commands).
Pushed then.
regards, tom lane
On Tue, Jan 31, 2023 at 4:00 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > The v3 patch LGTM (just for the logical replication commands). > > Pushed then. > Thanks for pushing the v3 patch. I'd forgotten about the 'streaming' option -- AFAIK this was previously a boolean parameter and so its [= value] part can also be omitted. However, in PG16 streaming became an enum type (on/off/parallel), and the value can still be omitted but that is not really being covered by the new generic text note about booleans added by yesterday's patch. e.g. The enum 'streaming' value part can still be omitted. test_sub=# create subscription sub1 connection 'host=localhost dbname=test_pub' publication pub1 with (streaming); Perhaps a small top-up patch to CREATE SUBSCRIPTION is needed to describe this special case? PSA. (I thought mentioning this special streaming case again for ALTER SUBSCRIPTION might be overkill) ------ Kind Regards, Peter Smith. Fujitsu Australia
Attachment
Peter Smith <smithpb2250@gmail.com> writes:
> I'd forgotten about the 'streaming' option -- AFAIK this was
> previously a boolean parameter and so its [= value] part can also be
> omitted. However, in PG16 streaming became an enum type
> (on/off/parallel), and the value can still be omitted but that is not
> really being covered by the new generic text note about booleans added
> by yesterday's patch.
Hmph. I generally think that options defined like this (it's a boolean,
except it isn't) are a bad idea, and would prefer to see that API
rethought while we still can.
regards, tom lane
On Tue, Jan 31, 2023 at 4:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > I'd forgotten about the 'streaming' option -- AFAIK this was > > previously a boolean parameter and so its [= value] part can also be > > omitted. However, in PG16 streaming became an enum type > > (on/off/parallel), and the value can still be omitted but that is not > > really being covered by the new generic text note about booleans added > > by yesterday's patch. > > Hmph. I generally think that options defined like this (it's a boolean, > except it isn't) are a bad idea, and would prefer to see that API > rethought while we still can. > We have discussed this during development and considered using a separate option like parallel = on (or say parallel_workers = n) but there were challenges with the same. See discussion in email [1]. We also checked that we have various other places using something similar for options. For example COPY commands option: HEADER [ boolean | MATCH ]. Then GUCs like synchronous_commit/constraint_exclusion/huge_pages/backslash_quote have similar values. Then some of the reloptions like buffering, vacuum_index_cleanup also have off/on/auto values. I think having an enum where off/on are present is already used. In this case, the main reason is that after discussion we felt it is better to have streaming as an enum with values off/on/parallel instead of introducing a new option. [1] - https://www.postgresql.org/message-id/CAA4eK1Kt67RdW0WTR-LTxasj3pyukPCYhfA0arDUNnsz2wh03A%40mail.gmail.com -- With Regards, Amit Kapila.
Amit Kapila <amit.kapila16@gmail.com> writes:
> On Tue, Jan 31, 2023 at 4:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hmph. I generally think that options defined like this (it's a boolean,
>> except it isn't) are a bad idea, and would prefer to see that API
>> rethought while we still can.
> We have discussed this during development and considered using a
> separate option like parallel = on (or say parallel_workers = n) but
> there were challenges with the same. See discussion in email [1]. We
> also checked that we have various other places using something similar
> for options. For example COPY commands option: HEADER [ boolean |
> MATCH ].
Yeah, and it's bad experiences with the existing cases that make me
not want to add more. Every one of those was somebody taking the
easy way out. It generally leads to parsing oddities, such as
not accepting all the same spellings of "boolean" as before.
regards, tom lane
On Tuesday, January 31, 2023 10:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: Hi, > Amit Kapila <amit.kapila16@gmail.com> writes: > > On Tue, Jan 31, 2023 at 4:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Hmph. I generally think that options defined like this (it's a > >> boolean, except it isn't) are a bad idea, and would prefer to see > >> that API rethought while we still can. > > > We have discussed this during development and considered using a > > separate option like parallel = on (or say parallel_workers = n) but > > there were challenges with the same. See discussion in email [1]. We > > also checked that we have various other places using something similar > > for options. For example COPY commands option: HEADER [ boolean | > > MATCH ]. > > Yeah, and it's bad experiences with the existing cases that make me not want to > add more. Every one of those was somebody taking the easy way out. It > generally leads to parsing oddities, such as not accepting all the same spellings > of "boolean" as before. I understand the worry of parsing oddities. I think we have tried to make the streaming option keep accepting all the same spellings of boolean(e.g. the option still accept(1/0/true/false...)). And this is similar to some other option like COPY HEADER option which accepts all the boolean value and the 'match' value. Some other GUCs like wal_compression also behave similarly: 0/1/true/false/on/off/lz1/pglz are all valid values. Best Regards, Hou zj
On Mon, Jan 30, 2023 at 8:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Zheng Li <zhengli10@gmail.com> writes: > > The behavior is due to the following code > > https://github.com/postgres/postgres/blob/master/src/backend/commands/define.c#L113 > > Yeah, so you can grep for places that have this behavior by looking > for defGetBoolean calls ... and there are quite a few. That leads > me to the conclusion that we'd better invent a fairly stylized > documentation solution that we can plug into a lot of places, > rather than thinking of slightly different ways to say it and > places to say it. I'm not necessarily opposed to Peter's desire > to fix replication-related commands first, but we have more to do > later. > > I'm also not that thrilled with putting the addition up at the top > of the relevant text. This behavior is at least two decades old, > so if we've escaped documenting it at all up to now, it can't be > that important to most people. > > I also notice that ALTER SUBSCRIPTION has fully three different > sub-sections with about equal claims on this note, if we're going > to stick it directly into the affected option lists. > > That all leads me to propose that we add the new text at the end of > the Parameters <refsect1> in the affected man pages. So about > like the attached. (I left out alter_publication.sgml, as I'm not > sure it needs its own copy of this text --- it doesn't describe > individual parameters at all, just refer to CREATE PUBLICATION.) > > regards, tom lane > Hi, Here is a similar update for another page: "55.4 Streaming Replication Protocol" [0]. This patch was prompted by a review comment reply at [1] (#2). I've used text almost the same as the boilerplate text added by the previous commit [2] ~ PSA patch v4. ====== [0] https://www.postgresql.org/docs/devel/protocol-replication.html [1] https://www.postgresql.org/message-id/OS0PR01MB571663BCE8B28597D462FADE946A2%40OS0PR01MB5716.jpnprd01.prod.outlook.com [2] https://github.com/postgres/postgres/commit/ec7e053a98f39a9e3c7e6d35f0d2e83933882399 Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Fri, Jan 12, 2024 at 4:07 PM Peter Smith <smithpb2250@gmail.com> wrote: > > On Mon, Jan 30, 2023 at 8:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Zheng Li <zhengli10@gmail.com> writes: > > > The behavior is due to the following code > > > https://github.com/postgres/postgres/blob/master/src/backend/commands/define.c#L113 > > > > Yeah, so you can grep for places that have this behavior by looking > > for defGetBoolean calls ... and there are quite a few. That leads > > me to the conclusion that we'd better invent a fairly stylized > > documentation solution that we can plug into a lot of places, > > rather than thinking of slightly different ways to say it and > > places to say it. I'm not necessarily opposed to Peter's desire > > to fix replication-related commands first, but we have more to do > > later. > > > > I'm also not that thrilled with putting the addition up at the top > > of the relevant text. This behavior is at least two decades old, > > so if we've escaped documenting it at all up to now, it can't be > > that important to most people. > > > > I also notice that ALTER SUBSCRIPTION has fully three different > > sub-sections with about equal claims on this note, if we're going > > to stick it directly into the affected option lists. > > > > That all leads me to propose that we add the new text at the end of > > the Parameters <refsect1> in the affected man pages. So about > > like the attached. (I left out alter_publication.sgml, as I'm not > > sure it needs its own copy of this text --- it doesn't describe > > individual parameters at all, just refer to CREATE PUBLICATION.) > > > > regards, tom lane > > > > Hi, > > Here is a similar update for another page: "55.4 Streaming Replication > Protocol" [0]. This patch was prompted by a review comment reply at > [1] (#2). > > I've used text almost the same as the boilerplate text added by the > previous commit [2] > > ~ > > PSA patch v4. > > ====== > [0] https://www.postgresql.org/docs/devel/protocol-replication.html > [1] https://www.postgresql.org/message-id/OS0PR01MB571663BCE8B28597D462FADE946A2%40OS0PR01MB5716.jpnprd01.prod.outlook.com > [2] https://github.com/postgres/postgres/commit/ec7e053a98f39a9e3c7e6d35f0d2e83933882399 > > Kind Regards, > Peter Smith. > Fujitsu Australia Bump. Kind Regards, Peter Smith. Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
> Here is a similar update for another page: "55.4 Streaming Replication
> Protocol" [0]. This patch was prompted by a review comment reply at
> [1] (#2).
> I've used text almost the same as the boilerplate text added by the
> previous commit [2]
Pushed, except I put it at the bottom of the section not the top.
regards, tom lane