Re: [16+] subscription can end up in inconsistent state - Mailing list pgsql-bugs

Hi Vignesh. Here are some review comments for the latest patch v3-0001

======
Commit message

1.
Password can be specified from PGPASS file or PGPASSWORD environment,
when password required is true then for non-superusers we should make sure that
the password option is provided from connection string.

~~

SUGGESTION
When the 'password_required' subscription parameter is true, a
non-superuser is only allowed to specify the password via the
connection string, not using a PGPASS file or PGPASSWORD environment
variable.

======
GENERAL

2. PG DOCS

Currently the CREATE SUBSCRIPTION [1] notes say:

password_required (boolean)
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
true. Only superusers can set this value to false.

~

Should there be more information here to say (when 'password_required'
is true) *how* non-superusers are required to specify the password?

~~~

3. Connection string option 'passfile'?

IIUC the subscription "password_required" option means that the
password must be in the connection string. I'm not sure how the other
connection string "passfile" option [2] fits into this rule. The
current patch looks like it would reject the passfile option (because
IIUC libpqrcv_check_conninfo only looks for "password") -- is it
deliberate? And, should it be documented/commented somewhere?

======
src/backend/replication/libpqwalreceiver/libpqwalreceiver.c

4.
+ /*
+ * Check if the password is specified as part of connection string itself
+ * for non-superusers. This check is required to prevent password being
+ * set from PGPASSWORD environment or PGPASS file in case of non-superusers.
+ */
+ if (must_use_password)
+ libpqrcv_check_conninfo(conninfo, true);
+

Consider using the same wording in the comment as the suggested commit message.


======
src/test/subscription/t/027_nosuperuser.pl

5.
+# If the superuser owned subscription which was using a connection string
+# (without password) with the password coming from the PGPASSWORD environment
+# or PGPASS FILE transfers ownership to a non-superuser, then the next
+# subscription command(which connects to the publisher) should fail with
+# password required error.

Below is my para-phrasing of the comment. Choose whichever you like
best, but if you keep the original please fix the spaces.

SUGGESTION
If the subscription connection requires a password ('password
required' = true) then a non-superuser must specify that password in
the connection string.

This test starts out with a superuser-owned subscription having a
connection string lacking a password -- instead, the password is
coming from the PGPASSWORD environment or PGPASS FILE. Subscription
ownership is then transferred to a non-superuser. The next
subscription command (which connects to the publisher) should fail
with a password required error.

~~~

6. GENERAL - improved names in the test...

a. I felt it might be clearer to name things slightly differently.
b. Perhaps you also wanted to prefix the pub/sub names with 'regress_'

So,

regress_test ==> regress_test_user

PUBLICATION test ==> regress_test_pub

SUBSCRIPTION test_sub ==> regress_test_sub

~~~

7.
+$node_subscriber1->safe_psql(
+ 'postgres', qq(
+CREATE SUBSCRIPTION test_sub CONNECTION '$publisher_connstr1'
PUBLICATION test WITH (enabled=false);
+));
+
+$node_subscriber1->safe_psql('postgres',
+ qq(ALTER SUBSCRIPTION test_sub ENABLE;));

Why does the test create the subscription with enabled=false only to
immediately enable it on the next line?

~~~

8.
+# Non-superuser must specify password in the connection string
+my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
+ 'postgres', qq(
+SET SESSION AUTHORIZATION regress_test;
+ALTER SUBSCRIPTION test_sub REFRESH PUBLICATION;
+));
+isnt($ret, 0,
+ "non zerof exit for subscription whose owner is a non-superuser must
specify password through connection string"
+);
+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'
+);

8a.
+# Non-superuser must specify password in the connection string

Refer to my prior question -- what about the 'passfile' connection
string option?

~

8b.
typo /zerof/zero/

~~~

9. what about positive test?

I felt this test should conclude with you changing the connection
string so it *does* include the secret password, to demonstrate the
non-superuser connecting successfully.

======
[1] https://www.postgresql.org/docs/current/sql-createsubscription.html
[2] https://www.postgresql.org/docs/devel/libpq-connect.html#LIBPQ-CONNECT-PASSFILE

Kind Regards,
Peter Smith.
Fujitsu Australia