On Tue, Mar 10, 2026 at 10:25 AM Amit Kapila <[email protected]> wrote:
>
> On Mon, Mar 9, 2026 at 4:23 PM vignesh C <[email protected]> wrote:
> >
> > This is addressed in the v60 version patch attached.
> > Also Nisha's comments from [1] and Shveta's comments from [2] are
> > handled in the v60 version.
> >
>
> *
> + /* Check that user is allowed to manipulate the publication. */
> + if (excepttables && !pubform->puballtables)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("publication \"%s\" is not defined as FOR ALL TABLES",
> + NameStr(pubform->pubname)),
> + errdetail("Except tables cannot be added to or dropped from
> publications that are not defined as FOR ALL TABLES."));
> +
> + /*
> + * SET ALL TABLES is allowed only for publications defined as FOR ALL
> + * TABLES.
> + */
> + if (stmt->action == AP_SetObjects && !pubform->puballtables &&
> + !tables && !excepttables && !schemaidlist)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("publication \"%s\" is not defined as FOR ALL TABLES",
> + NameStr(pubform->pubname)));
> +
> + /* Only a superuser can add or remove EXCEPT tables */
> + if (stmt->action == AP_SetObjects &&
> + (excepttables || (!tables && !excepttables && !schemaidlist)) &&
> + !superuser())
> + ereport(ERROR,
> + errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("must be superuser to add or drop except tables"));
>
> All the above checks are trying to drive the answer to whether the new
> Alter statement is for_all_tables via indirect checks. Why not keep
> for_all_tables boolean variable in AlterPublicationStmt similar to
> CreatePublicationStmt? That should simplify the above checks.
>
+1
> * 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?
Thanks
Shveta
