| Lists: | pgsql-hackers |
|---|
| From: | Marti Raudsepp <marti(at)juffo(dot)org> |
|---|---|
| To: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | [PATCH] Cleanup: unify checks for catalog modification |
| Date: | 2014-10-14 22:56:27 |
| Message-ID: | CABRT9RB3AiULxzeTUZ-c-wpYdvfMGbxG4fPne38vtRNdm86Svw@mail.gmail.com |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
Hi list,
I happened to notice that there are no less than 14 places in the code
that check whether a relation is a system catalog and throwing the
error "permission denied: "foo" is a system catalog"
The attached patch factors all of those into a single
ForbidSystemTableMods() function. Is this considered an improvement?
Should I add it to CommitFest?
Regards,
Marti
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-Cleanup-unify-checks-for-catalog-modification.patch | binary/octet-stream | 10.0 KB |
| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | Marti Raudsepp <marti(at)juffo(dot)org> |
| Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: [PATCH] Cleanup: unify checks for catalog modification |
| Date: | 2014-10-14 23:10:35 |
| Message-ID: | 28734.1413328235@sss.pgh.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
Marti Raudsepp <marti(at)juffo(dot)org> writes:
> I happened to notice that there are no less than 14 places in the code
> that check whether a relation is a system catalog and throwing the
> error "permission denied: "foo" is a system catalog"
> The attached patch factors all of those into a single
> ForbidSystemTableMods() function. Is this considered an improvement?
I'd argue not. The code bulk savings is minimal, and this change
would degrade the usefulness of the file/line number reporting that's
built into ereport(). Admittedly it's a judgment call --- we've certainly
built error-reporting subroutines in cases where a significant amount of
complexity could be folded into the subroutine. But I don't think this
case meets the threshold.
regards, tom lane
| From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | Marti Raudsepp <marti(at)juffo(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: [PATCH] Cleanup: unify checks for catalog modification |
| Date: | 2014-10-15 02:00:09 |
| Message-ID: | 20141015020009.GH7043@eldon.alvh.no-ip.org |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Lists: | pgsql-hackers |
Tom Lane wrote:
> Marti Raudsepp <marti(at)juffo(dot)org> writes:
> > I happened to notice that there are no less than 14 places in the code
> > that check whether a relation is a system catalog and throwing the
> > error "permission denied: "foo" is a system catalog"
>
> > The attached patch factors all of those into a single
> > ForbidSystemTableMods() function. Is this considered an improvement?
>
> I'd argue not. The code bulk savings is minimal, and this change
> would degrade the usefulness of the file/line number reporting that's
> built into ereport().
Along these lines, I've sometimes thought that it could be useful to
pass down file/line info from certain callers down to certain generic
check subroutines such as the one being proposed here. (I can't recall
any specific examples offhand.) Of course, doing it manually would be
very tedious and error prone, but perhaps we could have something like a
macro system that both sets up arguments in the called function, and
sets up the values in the callee.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services