Re: alter table set TABLE ACCESS METHOD

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Jeff Davis <pgsql(at)j-davis(dot)com>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org, Jacob Champion <pchampion(at)vmware(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Ashwin Agrawal <aagrawal(at)pivotal(dot)io>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
Subject: Re: alter table set TABLE ACCESS METHOD
Date: 2021-07-29 06:27:35
Message-ID: YQJKV29/
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Jul 28, 2021 at 01:05:10PM -0700, Jeff Davis wrote:
> On Wed, 2021-07-28 at 14:02 +0900, Michael Paquier wrote:
>> Right. Isn't that an older issue though? A rewrite involved after a
>> change of relpersistence does not call the hook either. It looks to
>> me that this should be after finish_heap_swap() to match with
>> ATExecSetTableSpace() in ATRewriteTables(). The only known user of
>> object_access_hook in the core code is sepgsql, so this would
>> involve a change of behavior. And I don't recall any backpatching
>> that added a post-alter hook.
> Sounds like it should be a different patch. Thank you.

Doing any checks around the hooks of objectaccess.h is very annoying,
because we have no modules to check after them easily except sepgsql.
Anyway, I have been checking that, with the hack-ish module attached
and tracked down that swap_relation_files() calls
InvokeObjectPostAlterHookArg() already (as you already spotted?), but
that's an internal change when it comes to SET LOGGED/UNLOGGED/ACCESS

Attached is a small module I have used for those tests, for
reference. It passes on HEAD, and with the patch attached you can see
the extra entries.

>>> Also, I agree with Justin that it should fail when there are
>>> multiple
>>> SET ACCESS METHOD subcommands consistently, regardless of whether
>>> one
>>> is a no-op, and it should probably throw a syntax error to match
>>> SET
>> Hmm. Okay.

I'd still disagree with that. One example is SET LOGGED that would
not fail when repeated multiple times. Also, tracking down if a SET
ACCESS METHOD subcommand has been passed down requires an extra field
in each tab entry so I am not sure that this is worth the extra

I can see benefits and advantages one way or the other, and I would
tend to keep the code a maximum simple as we never store InvalidOid
for a table AM. Anyway, I won't fight the majority either.

>>> Minor nit: in tab-complete.c, why does it say "<smt>"? Is that just
>>> a
>>> typo or is there a reason it's different from everything else,
>>> which
>>> uses "<sth>"? And what does "sth" mean anyway?
>> "Something". That should be "<sth>" to be consistent with the area.
> These two issues are pretty minor.

Fixed this one, while not forgetting about it. Thanks.

Attachment Content-Type Size
object_hooks.tar.gz application/gzip 2.2 KB
alter-table-hook.patch text/x-diff 456 bytes

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Smith 2021-07-29 07:01:52 Re: Out-of-memory error reports in libpq
Previous Message Masahiko Sawada 2021-07-29 05:47:35 Re: Skipping logical replication transactions on subscriber side