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

On Fri, 22 Sept 2023 at 01:45, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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.

I thought of moving the passwordrequired && !superuser logic inside
libpq connect but it is not simplifying the code. Reverted those
changes and kept only the changes to check if the password is present
in the connection string in case of must_use_password.

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

We can have this check before the connection, so this can be removed.

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

I will post these changes in a separate email.
The attached v2 version patch has the changes for the same.

Regards,
Vignesh