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

From: Tom Lane <[email-masked]>
Date: Mon, 30 Jan 2023 12:00:31 -0500

Peter Smith <[email-masked]> writes:
> The v3 patch LGTM (just for the logical replication commands).

Pushed then.

			regards, tom lane

---

From: Peter Smith <[email-masked]>
Date: Tue, 31 Jan 2023 09:36:53 +1100

> > The v3 patch LGTM (just for the logical replication commands).

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

---

From: Tom Lane <[email-masked]>
Date: Mon, 30 Jan 2023 17:55:43 -0500

> really being covered by the new generic text note about booleans added

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 31 Jan 2023 11:12:24 +0530

> 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.

---

From: Tom Lane <[email-masked]>
Date: Tue, 31 Jan 2023 09:49:03 -0500

> 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

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 1 Feb 2023 06:49:24 +0000

On Tuesday, January 31, 2023 10:49 PM Tom Lane <[email-masked]> wrote:

Hi,

> >> 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.
> 
> 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