Обсуждение: CREATE SUBSCRIPTION -- add missing tab-completes
There are some recent comment that added new options for CREATE SUBSCRIPTION "Add new predefined role pg_create_subscription." [1] This added a new "password_required" option. "Add a run_as_owner option to subscriptions." [2] This added a "run_as_owner" option. ~~ AFAICT the associated tab-complete code was accidentally omitted. PSA patches to add those tab completions. ------ [1] https://github.com/postgres/postgres/commit/c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6 [2] https://github.com/postgres/postgres/commit/482675987bcdffb390ae735cfd5f34b485ae97c6 Kind Regards, Peter Smith. Fujitsu Australia
Вложения
On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > There are some recent comment that added new options for CREATE SUBSCRIPTION > ... > PSA patches to add those tab completions. > LGTM, so pushed. BTW, while looking at this, I noticed that newly added options "password_required" and "run_as_owner" has incorrectly mentioned their datatype as a string in the docs. It should be boolean. I think "password_required" belongs to first section of docs which says: "The following parameters control what happens during subscription creation". The attached patch makes those changes. What do you think? -- With Regards, Amit Kapila.
Вложения
On Fri, Apr 7, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > There are some recent comment that added new options for CREATE SUBSCRIPTION > > > ... > > PSA patches to add those tab completions. > > > > LGTM, so pushed. BTW, while looking at this, I noticed that newly > added options "password_required" and "run_as_owner" has incorrectly > mentioned their datatype as a string in the docs. It should be > boolean. +1 > I think "password_required" belongs to first section of docs > which says: "The following parameters control what happens during > subscription creation". But the documentation of ALTER SUBSCRIPTION says: The parameters that can be altered are slot_name, synchronous_commit, binary, streaming, disable_on_error, password_required, run_as_owner, and origin. Only a superuser can set password_required = false. ISTM that both password_required and run_as_owner are parameters to control the subscription's behavior, like disable_on_error and streaming. So it looks good to me that password_required belongs to the second section. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On Fri, Apr 7, 2023 at 1:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Fri, Apr 7, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > > LGTM, so pushed. BTW, while looking at this, I noticed that newly > > added options "password_required" and "run_as_owner" has incorrectly > > mentioned their datatype as a string in the docs. It should be > > boolean. > > +1 > > > I think "password_required" belongs to first section of docs > > which says: "The following parameters control what happens during > > subscription creation". > > But the documentation of ALTER SUBSCRIPTION says: > > The parameters that can be altered are slot_name, synchronous_commit, > binary, streaming, disable_on_error, password_required, run_as_owner, > and origin. Only a superuser can set password_required = false. > By the above, do you intend to say that all the parameters that can be altered are in the second list? If so, slot_name belongs to the first category. > ISTM that both password_required and run_as_owner are parameters to > control the subscription's behavior, like disable_on_error and > streaming. So it looks good to me that password_required belongs to > the second section. > Do you mean that because 'password_required' is used each time we make a connection to a publisher during replication, it should be in the second category? If so, slot_name is also used during the start replication each time. BTW, do we need to check one or both of these parameters in maybe_reread_subscription() where we "Exit if any parameter that affects the remote connection was changed." -- With Regards, Amit Kapila.
On Friday, April 7, 2023 5:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Apr 7, 2023 at 1:12 PM Masahiko Sawada <sawada.mshk@gmail.com> > wrote: > > > > On Fri, Apr 7, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> > wrote: > > > > > > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250@gmail.com> > wrote: > > > > > > > > > > LGTM, so pushed. BTW, while looking at this, I noticed that newly > > > added options "password_required" and "run_as_owner" has incorrectly > > > mentioned their datatype as a string in the docs. It should be > > > boolean. > > > > +1 > > > > > I think "password_required" belongs to first section of docs which > > > says: "The following parameters control what happens during > > > subscription creation". > > > > But the documentation of ALTER SUBSCRIPTION says: > > > > The parameters that can be altered are slot_name, synchronous_commit, > > binary, streaming, disable_on_error, password_required, run_as_owner, > > and origin. Only a superuser can set password_required = false. > > > > By the above, do you intend to say that all the parameters that can be altered > are in the second list? If so, slot_name belongs to the first category. > > > ISTM that both password_required and run_as_owner are parameters to > > control the subscription's behavior, like disable_on_error and > > streaming. So it looks good to me that password_required belongs to > > the second section. > > > > Do you mean that because 'password_required' is used each time we make a > connection to a publisher during replication, it should be in the second > category? If so, slot_name is also used during the start replication each time. > > BTW, do we need to check one or both of these parameters in > maybe_reread_subscription() where we "Exit if any parameter that affects the > remote connection was changed." I think changing run_as_owner doesn't require to be checked as it only affect the role to perform the apply. But it seems password_required need to be checked in maybe_reread_subscription() because we used this parameter for connection. Best Regards, Hou zj
On Fri, Apr 7, 2023 at 6:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Apr 7, 2023 at 1:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Fri, Apr 7, 2023 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > > > > > LGTM, so pushed. BTW, while looking at this, I noticed that newly > > > added options "password_required" and "run_as_owner" has incorrectly > > > mentioned their datatype as a string in the docs. It should be > > > boolean. > > > > +1 > > > > > I think "password_required" belongs to first section of docs > > > which says: "The following parameters control what happens during > > > subscription creation". > > > > But the documentation of ALTER SUBSCRIPTION says: > > > > The parameters that can be altered are slot_name, synchronous_commit, > > binary, streaming, disable_on_error, password_required, run_as_owner, > > and origin. Only a superuser can set password_required = false. > > > > By the above, do you intend to say that all the parameters that can be > altered are in the second list? If so, slot_name belongs to the first > category. > > > ISTM that both password_required and run_as_owner are parameters to > > control the subscription's behavior, like disable_on_error and > > streaming. So it looks good to me that password_required belongs to > > the second section. > > > > Do you mean that because 'password_required' is used each time we make > a connection to a publisher during replication, it should be in the > second category? If so, slot_name is also used during the start > replication each time. I think that parameters used by the backend process when performing CREATE SUBSCRIPTION belong to the first category. And other parameters used by apply workers and tablesync workers belong to the second category. Since slot_name is used by both I'm not sure it should be in the second category, but password_requried seems to be used by only apply workers and tablesync workers, so it should be in the second category. > > BTW, do we need to check one or both of these parameters in > maybe_reread_subscription() where we "Exit if any parameter that > affects the remote connection was changed." As for run_as_owner, since we can dynamically switch the behavior I think we don't need to reconnect. I'm not really sure about password_required. From the implementation point of view, we don't need to reconnect. Even if password_required is changed from false to true, the apply worker already has the established connection. If it's changed from true to false, we might not want to reconnect. I think we need to consider it from the security point of view while checking the motivation that password_required was introduced. So probably it's better to discuss it on the original thread. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On Fri, Apr 7, 2023 at 6:59 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Fri, Apr 7, 2023 at 6:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Fri, Apr 7, 2023 at 1:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > Do you mean that because 'password_required' is used each time we make > > a connection to a publisher during replication, it should be in the > > second category? If so, slot_name is also used during the start > > replication each time. > > I think that parameters used by the backend process when performing > CREATE SUBSCRIPTION belong to the first category. And other parameters > used by apply workers and tablesync workers belong to the second > category. Since slot_name is used by both I'm not sure it should be in > the second category, but password_requried seems to be used by only > apply workers and tablesync workers, so it should be in the second > category. > Hmm, don't we use the option "password_requried" during CREATE SUBSCRIPTION when we have to connect? The second category is more about parameters that define the replication behavior so it is not clear to me how this falls in that category. See the initial discussion which led to the current situation [1]. Anyway, for now, I have just committed the fix for the datatype as we have not reached a consensus on this one. > > > > BTW, do we need to check one or both of these parameters in > > maybe_reread_subscription() where we "Exit if any parameter that > > affects the remote connection was changed." > > As for run_as_owner, since we can dynamically switch the behavior I > think we don't need to reconnect. I'm not really sure about > password_required. From the implementation point of view, we don't > need to reconnect. Even if password_required is changed from false to > true, the apply worker already has the established connection. If it's > changed from true to false, we might not want to reconnect. I think we > need to consider it from the security point of view while checking the > motivation that password_required was introduced. So probably it's > better to discuss it on the original thread. > Agreed and responded to the original thread [2]. [1] - https://www.postgresql.org/message-id/CAA4eK1Kmu74xHk2jcHTmKq8HBj3xK6n%3DRfiJB6dfV5zVSqqiFg%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CAA4eK1%2Bz9UDFEynXLsWeMMuUZc1iQkRwj2HNDtxUHTPo-u1F4A%40mail.gmail.com -- With Regards, Amit Kapila.
On Fri, 7 Apr 2023 at 01:28, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > PSA patches to add those tab completions. > > LGTM, so pushed. I moved this to the next CF but actually I just noticed the thread starts with the original patch being pushed. Maybe we should just close the CF entry? Is this further change relevant? -- Gregory Stark As Commitfest Manager
On Sun, Apr 9, 2023 at 11:33 AM Gregory Stark (as CFM) <stark.cfm@gmail.com> wrote: > > On Fri, 7 Apr 2023 at 01:28, Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Wed, Apr 5, 2023 at 5:58 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > PSA patches to add those tab completions. > > > > LGTM, so pushed. > > I moved this to the next CF but actually I just noticed the thread > starts with the original patch being pushed. Maybe we should just > close the CF entry? Is this further change relevant? > I have closed the CF entry as the patch for which this entry was created is already committed. If anything comes as a result of further discussion, I'll take care of it, or if required we can start a separate thread. -- With Regards, Amit Kapila.
On Fri, Apr 7, 2023 at 9:29 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > I think that parameters used by the backend process when performing > CREATE SUBSCRIPTION belong to the first category. And other parameters > used by apply workers and tablesync workers belong to the second > category. Since slot_name is used by both I'm not sure it should be in > the second category, but password_requried seems to be used by only > apply workers and tablesync workers, so it should be in the second > category. I agree. I think actually the current division is quite odd. The only parameters that strictly affect the CREATE SUBSCRIPTION command are "connect" and "create_slot". "enabled" and "slot_name" clearly control later behavior, because you can alter both of them later, with ALTER SUBSCRIPTION! The "enabled" parameter is changed using different syntax, ALTER SUBSCRIPTION .. ENABLE | DISABLE instead of ALTER SUBSCRIPTION ... SET (enabled = true | false), which is possibly not the best choice, but regardless of that, these parameters clearly affect behavior later, not just at CREATE SUBSCRIPTION time. Probably we ought to just collapse the sections together somehow, and use the text to clarify the exact behavior as required. I definitely disagree with the idea of moving the new parameters to the other section -- that's clearly wrong. -- Robert Haas EDB: http://www.enterprisedb.com