On Tue, Mar 10, 2026 at 11:16 AM shveta malik <[email protected]> wrote: > > On Tue, Mar 10, 2026 at 10:25 AM Amit Kapila <[email protected]> wrote: > > > > > * Also, the superuser check should be earlier in the code, say just > > after following existing check: > > if ((stmt->action == AP_AddObjects || stmt->action == AP_SetObjects) && > > schemaidlist && !superuser()) > > ereport(ERROR, > > (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > > errmsg("must be superuser to add or set schemas"))); > > > > I too thought about it, but then it seems the current order of check > gives better error (IMO). As an example consider where pub1 is a > publication for explicit tables, then doing 'ALTER PUB SET ALL TABLES' > on it should give the error that 'pub1 is not for ALL TABLES' rather > than 'must be superuser'. Because even if ownership was correct, it > was not going to work. > > Currently: > > postgres=> alter publication pub1 set all tables; > ERROR: publication "pub1" is not defined as FOR ALL TABLES > > And for pub2 which is for all tables, we get user error: > > postgres=> alter publication pub2 set all tables; > ERROR: must be superuser to add or drop except tables > > The errors in both cases seem correct to me. What do you think? >
I still feel superuser check should be first and the user should get that when it tries to use FOR ALL TABLES. Also, the better error message for the same would be: "must be superuser to define FOR ALL TABLES publication" -- With Regards, Amit Kapila.
