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

On Thu, Sep 21, 2023 at 3:48 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> Attached patch has the changes for the same.

Almost everything about this patch seems incorrect to me.

It seems to rip out all of the must_use_password = passwordrequired &&
!superuser logic, which is not at all what was being discussed here,
and which I think is not desirable.

And it does stuff like this:

@@ -275,10 +288,18 @@ libpqrcv_check_conninfo(const char *conninfo,
bool must_use_password)
}

if (!uses_password)
+ {
+ if (conn)
+ {
+ libpqsrv_disconnect(conn->streamConn);
+ pfree(conn);
+ }
+
ereport(ERROR,
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
errmsg("password is required"),
errdetail("Non-superusers must provide a password in the connection
string.")));
+ }
}

PQconninfoFree(opts);

There are zero comments explaining what this is supposed to
accomplish, but I don't think it's any of the things discussed on this
thread.

+ CacheRegisterSyscacheCallback(AUTHOID,
+ subscription_change_cb,
+ (Datum) 0);

I think if we want to do this it should be a separate patch from
adding the additional error checks. And I think it should be
accompanied by a comment update.

--
Robert Haas
EDB: http://www.enterprisedb.com