pub/sub - specifying optional parameters without values.

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

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

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

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

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

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]/messages/by-id/CAA4eK1Kt67RdW0WTR-LTxasj3pyukPCYhfA0arDUNnsz2wh03A@mail.gmail.com. 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]: /messages/by-id/CAA4eK1Kt67RdW0WTR-LTxasj3pyukPCYhfA0arDUNnsz2wh03A@mail.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]https://www.postgresql.org/docs/devel/protocol-replication.html. This patch was prompted by a review comment reply at
[1]: /messages/by-id/OS0PR01MB571663BCE8B28597D462FADE946A2@OS0PR01MB5716.jpnprd01.prod.outlook.com

I've used text almost the same as the boilerplate text added by the
previous commit [2]https://github.com/postgres/postgres/commit/ec7e053a98f39a9e3c7e6d35f0d2e83933882399

~

PSA patch v4.

======
[0]: https://www.postgresql.org/docs/devel/protocol-replication.html
[1]: /messages/by-id/OS0PR01MB571663BCE8B28597D462FADE946A2@OS0PR01MB5716.jpnprd01.prod.outlook.com
[2]: https://github.com/postgres/postgres/commit/ec7e053a98f39a9e3c7e6d35f0d2e83933882399

Kind Regards,
Peter Smith.
Fujitsu Australia

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] /messages/by-id/OS0PR01MB571663BCE8B28597D462FADE946A2@OS0PR01MB5716.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