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

On Thu, Sep 21, 2023 at 3:48 PM vignesh C <vignesh21@gmail.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