Re: [16+] subscription can end up in inconsistent state

On Fri, 6 Oct 2023 at 06:43, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for v4-0001
>
> ======
> 1. GENERAL - terminology
>
> AFAICT from existing documentation, things like 'password' are
> referred to as "parameters" of the connection string (i.e. not
> "options" of the connection string), so most review comments below are
> just repetitions of this point to make all the wording consistent.

Modified

> ======
> Commit Message
>
> 2.
> When the 'password_required' subscription parameter is true, a
> non-superuser is only allowed to specify the password via the password
> option of connection string, not using a PGPASS file or PGPASSWORD
> environment variable.
>
> ~
>
> (change the term and add the missing word 'the')
>
> /the password option of connection string/the password parameter of
> the connection string/

Modified

> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 3.
>           <para>
>            Specifies whether connections to the publisher made as a result
> -          of this subscription must use password authentication. This setting
> -          is ignored when the subscription is owned by a superuser.
> -          The default is <literal>true</literal>. Only superusers can set
> -          this value to <literal>false</literal>.
> +          of this subscription must use password authentication. If
> +          <literal>true</literal>, a non-superuser is only allowed to specify
> +          the password via the password option in connection string. This
> +          setting is ignored when the subscription is owned by a superuser. The
> +          default is <literal>true</literal>. Only superusers can set this
> +          value to <literal>false</literal>.
>           </para>
>
> 3a.
>
> (same wording as commit message)
>
> /the password option in connection string/the password parameter of
> the connection string/

Modified

> ~
>
> 3b.
> Also, consider using a link, or at least a <literal> for that
> "password" parameter.

Modified

> ======
> src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
>
> 4. libpqrcv_connect
>
> + /*
> + * A non-superuser is only allowed to specify the password via the
> + * password option in connection string, not using a PGPASS file or
> + * PGPASSWORD environment variable.
> + */
>
> (same wording as commit message)
>
> /the password option in connection string/the password parameter of
> the connection string/

Modified

> ======
> src/test/subscription/t/027_nosuperuser.pl
>
> 5.
> +isnt($ret, 0,
> + "non zero exit for subscription whose owner is a non-superuser must
> specify password through connection string"
> +);
>
> (same wording as commit message)
>
> /password through connection string/password parameter of the connection string/

Modified

> ~~~
>
> 6.
> +is( $stderr, 'psql:<stdin>:3: ERROR:  password is required
> +DETAIL:  Non-superusers must provide a password in the connection string.',
> + 'subscription whose owner is a non-superuser must specify password
> through connection string'
>
> (same wording as commit message)
>
> /password through connection string/password parameter of the connection string/

Modified

> ~~~
>
> 7.
> +# It should be successful after including the password in connection string
>
> /It should be successful/It should succeed/
>
> ~
>
> (same wording as commit message)
>
> /password in connection string/password parameter of the connection string/

Modified

> ~~~
>
> 8.
> +is($ret, 0,
> + "Non-superuser will be able to refresh the publication when the
> password is specified in connection string"
> +);
>
> (similar wording as commit message)
>
> /when the password is specified in connection string/after specifying
> the password parameter of the connection string/

Modified

The attached v5 version patch has the changes for the same.

Regards,
Vignesh