From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Jeff Davis <pgsql(at)j-davis(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Invalidate the subscription worker in cases where a user loses their superuser status |
Date: | 2023-10-03 00:39:09 |
Message-ID: | CAHut+Ps8aFyK3dizyaOJrQ8kwioG+LdqKfmOfAfG7y+naOJHTQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Some review comments for v5.
======
src/backend/catalog/pg_subscription.c
1. GetSubscription - comment
+ /* Get superuser for subscription owner */
+ sub->ownersuperuser = superuser_arg(sub->owner);
+
The comment doesn't seem very good.
SUGGESTION
/* Is the subscription owner a superuser? */
======
2. General - consistency
Below are the code fragments using the new Subscription field.
AlterSubscription_refresh:
must_use_password = !sub->ownersuperuser && sub->passwordrequired;
AlterSubscription:
walrcv_check_conninfo(stmt->conninfo, sub->passwordrequired &&
!sub->ownersuperuser);
LogicalRepSyncTableStart:
must_use_password = MySubscription->passwordrequired &&
!MySubscription->ownersuperuser;
run_apply_worker:
must_use_password = MySubscription->passwordrequired &&
!MySubscription->ownersuperuser;
~
It is not a difference caused by this patch, but since you are
modifying these lines anyway, I felt it would be better if all the
expressions were consistent. So, in AlterSubscription_refresh IMO it
would be better like:
BEFORE
must_use_password = !sub->ownersuperuser && sub->passwordrequired;
SUGGESTION
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
======
Other than those trivial things, v5 LGTM.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2023-10-03 02:11:15 | [PGDOCS] Inconsistent linkends to "monitoring" views. |
Previous Message | Vik Fearing | 2023-10-03 00:21:21 | Re: Allow deleting enumerated values from an existing enumerated data type |