Horton

Thread

Commits

  1. Doc: clarify behavior of boolean options in replication protocol commands.

  2. Doc: clarify behavior of boolean options in replication commands.

  1. pub/sub - specifying optional parameters without values.

    Peter Smith <smithpb2250@gmail.com> — 2022-10-14T08:54:37Z

    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

  2. Re: pub/sub - specifying optional parameters without values.

    Justin Pryzby <pryzby@telsasoft.com> — 2022-10-17T20:09:21Z

    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.sgml doc/src/sgml/ref/create_materialized_view.sgml doc/src/sgml/ref/create_publication.sgml doc/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

  3. Re: pub/sub - specifying optional parameters without values.

    Peter Smith <smithpb2250@gmail.com> — 2022-10-19T02:10:30Z

    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.sgml doc/src/sgml/ref/create_materialized_view.sgml doc/src/sgml/ref/create_publication.sgml doc/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

  4. Re: pub/sub - specifying optional parameters without values.

    Zheng Li <zhengli10@gmail.com> — 2023-01-09T17:37:05Z

    Hi,

This documentation change looks good to me. I verified in testing and in code that the value for boolean parameters in PUB/SUB commands 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

  5. Re: pub/sub - specifying optional parameters without values.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-29T21:36:39Z

    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

  6. Re: pub/sub - specifying optional parameters without values.

    Peter Smith <smithpb2250@gmail.com> — 2023-01-30T07:15:58Z

    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

  7. Re: pub/sub - specifying optional parameters without values.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-30T17:00:31Z

    Peter Smith <smithpb2250@gmail.com> writes:
> The v3 patch LGTM (just for the logical replication commands).

Pushed then.

			regards, tom lane

  8. Re: pub/sub - specifying optional parameters without values.

    Peter Smith <smithpb2250@gmail.com> — 2023-01-30T22:36:53Z

    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

  9. Re: pub/sub - specifying optional parameters without values.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-30T22:55:43Z

    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

  10. Re: pub/sub - specifying optional parameters without values.

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-31T05:42:24Z

    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.

  11. Re: pub/sub - specifying optional parameters without values.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-31T14:49:03Z

    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

  12. RE: pub/sub - specifying optional parameters without values.

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2023-02-01T06:49:24Z

    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

  13. Re: pub/sub - specifying optional parameters without values.

    Peter Smith <smithpb2250@gmail.com> — 2024-01-12T05:07:32Z

    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

  14. Re: pub/sub - specifying optional parameters without values.

    Peter Smith <smithpb2250@gmail.com> — 2024-03-02T23:59:40Z

    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

  15. Re: pub/sub - specifying optional parameters without values.

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-07T21:17:37Z

    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