Horton

Thread

Commits

  1. Allow specifying an access method for partitioned tables

  2. Fix regression test output of sepgsql

  3. Add call to object access hook at the end of table rewrite in ALTER TABLE

  4. Fix typo in tab-complete.c

  5. Add support for SET ACCESS METHOD in ALTER TABLE

  6. tableam: introduce table AM infrastructure.

  1. alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-02-28T22:25:30Z

    I thought this was a good idea, but didn't hear back when I raised it before.
I was motivated to finally look into it by Dilip's toast compression patch,
which also (can do) a table rewrite when changing a column's toast compression.

I called this "set TABLE access method" rather than just "set access method"
for the reasons given on the LIKE thread:
https://www.postgresql.org/message-id/20210119210331.GN8560@telsasoft.com

I've tested this with zedstore, but the lack of 2nd, in-core table AM limits
testing possibilities.  It also limits at least my own ability to reason about
the AM API.  For example, I was surprised to hear that toast is a concept
that's intended to be applied to AMs other than heap.
https://www.postgresql.org/message-id/flat/CA%2BTgmoYTuT4sRtviMLOOO%2B79VnDCpCNyy9rK6UZFb7KEAVt21w%40mail.gmail.com

I plan to add to CF - it seems like a minor addition, but may not be for v14.

https://www.postgresql.org/message-id/20190818193533.GL11185@telsasoft.com
On Sun, Aug 18, 2019 at 02:35:33PM -0500, Justin Pryzby wrote:
>  . What do you think about pg_restore --no-tableam; similar to
>    --no-tablespaces, it would allow restoring a table to a different AM:
>    PGOPTIONS='-c default_table_access_method=zedstore' pg_restore --no-tableam ./pg_dump.dat -d postgres
>    Otherwise, the dump says "SET default_table_access_method=heap", which
>    overrides any value from PGOPTIONS and precludes restoring to new AM.
...
>  . it'd be nice if there was an ALTER TABLE SET ACCESS METHOD, to allow
>    migrating data.  Otherwise I think the alternative is:
>       begin; lock t;
>       CREATE TABLE new_t LIKE (t INCLUDING ALL) USING (zedstore);
>       INSERT INTO new_t SELECT * FROM t;
>       for index; do CREATE INDEX...; done
>       DROP t; RENAME new_t (and all its indices). attach/inherit, etc.
>       commit;
>
>  . Speaking of which, I think LIKE needs a new option for ACCESS METHOD, which
>    is otherwise lost.

  2. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-03-01T02:16:36Z

    On Sun, Feb 28, 2021 at 04:25:30PM -0600, Justin Pryzby wrote:
> I called this "set TABLE access method" rather than just "set access method"
> for the reasons given on the LIKE thread:
> https://www.postgresql.org/message-id/20210119210331.GN8560@telsasoft.com

ALTER TABLE applies to a table (or perhaps a sequence, still..), so
that sounds a bit weird to me to add again the keyword "TABLE" for
that.

> I've tested this with zedstore, but the lack of 2nd, in-core table AM limits
> testing possibilities.  It also limits at least my own ability to reason about
> the AM API.
>
> For example, I was surprised to hear that toast is a concept that's
> intended to be applied to AMs other than heap.
> https://www.postgresql.org/message-id/flat/CA%2BTgmoYTuT4sRtviMLOOO%2B79VnDCpCNyy9rK6UZFb7KEAVt21w%40mail.gmail.com

What kind of advanced testing do you have in mind?  It sounds pretty
much enough to me for a basic patch to use the trick with heap2 as
your patch does.  That would be enough to be sure that the rewrite
happens and that data is still around.  If you are worrying about
recovery, a TAP test with an immediate stop of the server could
equally be used to check after the FPWs produced for the new
relfilenode during the rewrite.
--
Michael

  3. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-03-01T03:20:26Z

    On Mon, Mar 01, 2021 at 11:16:36AM +0900, Michael Paquier wrote:
> On Sun, Feb 28, 2021 at 04:25:30PM -0600, Justin Pryzby wrote:
> > I called this "set TABLE access method" rather than just "set access method"
> > for the reasons given on the LIKE thread:
> > https://www.postgresql.org/message-id/20210119210331.GN8560@telsasoft.com
> 
> ALTER TABLE applies to a table (or perhaps a sequence, still..), so
> that sounds a bit weird to me to add again the keyword "TABLE" for
> that.

I don't know if you're following along the toast compression patch -
Alvaro had suggested that instead of making a new catalog just for a handful of
tuples for compression types, to instead store them in pg_am, with a new
am_type='c'.  So I proposed a patch for
| CREATE TABLE .. (LIKE other INCLUDING ACCESS METHOD),
but then decided that it should say INCLUDING *TABLE* ACCESS METHOD, since
otherwise it was somewhat strange that it didn't include the compression access
methods (which have a separate LIKE option).

> > I've tested this with zedstore, but the lack of 2nd, in-core table AM limits
> > testing possibilities.  It also limits at least my own ability to reason about
> > the AM API.
> >
> > For example, I was surprised to hear that toast is a concept that's
> > intended to be applied to AMs other than heap.
> > https://www.postgresql.org/message-id/flat/CA%2BTgmoYTuT4sRtviMLOOO%2B79VnDCpCNyy9rK6UZFb7KEAVt21w%40mail.gmail.com
> 
> What kind of advanced testing do you have in mind?  It sounds pretty
> much enough to me for a basic patch to use the trick with heap2 as
> your patch does.  That would be enough to be sure that the rewrite
> happens and that data is still around.

The issue is that the toast data can be compressed, so it needs to be detoasted
before pushing it to the other AM, which otherwise may not know how to
decompress it.

If it's not detoasted, this works with "COMPRESSION lz4" (since zedstore
happens to know how to decompress it) but that's just an accident, and it fails
with when using pglz.  That's got to do with 2 non-core patches - when core has
only heap, then I don't see how something like this can be exercized.

postgres=# DROP TABLE t; CREATE TABLE t (a TEXT COMPRESSION pglz) USING heap; INSERT INTO t SELECT repeat(a::text,9999) FROM generate_series(1,99)a; ALTER TABLE t SET ACCESS METHOD zedstore; SELECT * FROM t;
DROP TABLE
CREATE TABLE
INSERT 0 99
ALTER TABLE
2021-02-28 20:50:42.653 CST client backend[14958] psql ERROR:  compressed lz4 data is corrupt
2021-02-28 20:50:42.653 CST client backend[14958] psql STATEMENT:  SELECT * FROM t;

-- 
Justin

  4. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-03-08T01:07:07Z

    On Mon, Mar 01, 2021 at 11:16:36AM +0900, Michael Paquier wrote:
> On Sun, Feb 28, 2021 at 04:25:30PM -0600, Justin Pryzby wrote:
> > I called this "set TABLE access method" rather than just "set access method"
> > for the reasons given on the LIKE thread:
> > https://www.postgresql.org/message-id/20210119210331.GN8560@telsasoft.com
> 
> ALTER TABLE applies to a table (or perhaps a sequence, still..), so
> that sounds a bit weird to me to add again the keyword "TABLE" for
> that.

This renames to use SET ACCESS METHOD (resolving a silly typo);
And handles the tuple slots more directly;
And adds docs and tab completion;

Also, since 8586bf7ed8889f39a59dd99b292014b73be85342:
|    For now it's not allowed to set a table AM for a partitioned table, as
|    we've not resolved how partitions would inherit that. Disallowing
|    allows us to introduce, if we decide that's the way forward, such a
|    behaviour without a compatibility break.

I propose that it should behave like tablespace for partitioned rels:
ca4103025dfe, 33e6c34c3267

-- 
Justin

  5. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-03-08T07:30:23Z

    On Sun, Mar 07, 2021 at 07:07:07PM -0600, Justin Pryzby wrote:
> This renames to use SET ACCESS METHOD (resolving a silly typo);
> And handles the tuple slots more directly;
> And adds docs and tab completion;
> 
> Also, since 8586bf7ed8889f39a59dd99b292014b73be85342:
> |    For now it's not allowed to set a table AM for a partitioned table, as
> |    we've not resolved how partitions would inherit that. Disallowing
> |    allows us to introduce, if we decide that's the way forward, such a
> |    behaviour without a compatibility break.
> 
> I propose that it should behave like tablespace for partitioned rels:
> ca4103025dfe, 33e6c34c3267

Sounds sensible from here.  Based on the patch at hand, changing the
AM of a partitioned table does nothing for the existing partitions,
and newly-created partitions would inherit the new AM assigned to its
parent.  pg_dump is handling things right.

From what I can see, the patch in itself is simple, with central parts
in swap_relation_files() to handle the rewrite and make_new_heap() to
assign the new correct AM.

+ * Since these have no storage the tablespace can be updated with a simple
+ * metadata only operation to update the tablespace.
+ */
+static void
+ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod
This comment still refers to tablespaces.

+   /*
+    * Record dependency on AM.  This is only required for relations
+    * that have no physical storage.
+    */
+   changeDependencyFor(RelationRelationId, RelationGetRelid(rel),
+           AccessMethodRelationId, oldrelam,
+           newAccessMethod);
And?  Relations with storage also require this dependency.

-           if (tab->newTableSpace)
+           if (OidIsValid(tab->newTableSpace))
You are right, but this is just a noise diff in this patch.

+       swaptemp = relform1->relam;
+       relform1->relam = relform2->relam;
+       relform2->relam = swaptemp;
swap_relation_files() holds the central logic, but I can see that no
comments of this routine have been updated (header, comment when
looking at valid relfilenode{1,2}).

It seems to me that 0002 and 0003 should just be merged together.

+               /* Need to detoast tuples when changing AM XXX: should
check if one AM is heap and one isn't? */
+               if (newrel->rd_rel->relam != oldrel->rd_rel->relam)
+               {
+                   HeapTuple htup = toast_build_flattened_tuple(oldTupDesc,
+                           oldslot->tts_values,
+                           oldslot->tts_isnull);
This toast issue is a kind of interesting one, and it seems rather
right to rely on toast_build_flattened_tuple() to decompress things if
both table AMs support toast with the internals of toast knowing what
kind of compression has been applied to the stored tuple, rather than
have the table AM try to extra a toast tuple by itself.  I wonder
whether we should have a table AM API to know what kind of compression
is supported for a given table AMs at hand, because there is no need
to flatten things if both the origin and the target match their
compression algos, which would be on HEAD to make sure that both the
origin and table AMs have toast (relation_toast_am).  Your patch,
here, would flatten each tuples as long as the table AMs don't 
match.  That can be made cheaper in some cases.
--
Michael

  6. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-03-08T08:56:58Z

    On Mon, Mar 08, 2021 at 04:30:23PM +0900, Michael Paquier wrote:
> This toast issue is a kind of interesting one, and it seems rather
> right to rely on toast_build_flattened_tuple() to decompress things if
> both table AMs support toast with the internals of toast knowing what
> kind of compression has been applied to the stored tuple, rather than
> have the table AM try to extra a toast tuple by itself.  I wonder
> whether we should have a table AM API to know what kind of compression
> is supported for a given table AMs at hand, because there is no need
> to flatten things if both the origin and the target match their
> compression algos, which would be on HEAD to make sure that both the
> origin and table AMs have toast (relation_toast_am).  Your patch,
> here, would flatten each tuples as long as the table AMs don't 
> match.  That can be made cheaper in some cases.

I actually have an idea for this one, able to test the decompression
-> insert path when rewriting a relation with a new AM: we could add a
dummy_table_am in src/test/modules/.  By design, this table AM acts as
a blackhole, eating any data we insert into it but print into the logs
the data candidate for INSERT.  When doing a heap -> dummy_table_am
rewrite, the logs would provide coverage with the data from the origin
toast table.  The opposite operation does not really matter, though it
could be tested.  In one of my trees, I have something already close
to that:
https://github.com/michaelpq/pg_plugins/tree/master/blackhole_am/
--
Michael

  7. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-05-06T20:10:53Z

    On Mon, 2021-03-08 at 16:30 +0900, Michael Paquier wrote:
> This toast issue is a kind of interesting one, and it seems rather
> right to rely on toast_build_flattened_tuple() to decompress things
> if
> both table AMs support toast with the internals of toast knowing what
> kind of compression has been applied to the stored tuple, rather than
> have the table AM try to extra a toast tuple by itself.  I wonder
> whether we should have a table AM API to know what kind of
> compression
> is supported for a given table AMs at hand, because there is no need
> to flatten things if both the origin and the target match their
> compression algos, which would be on HEAD to make sure that both the
> origin and table AMs have toast (relation_toast_am).  Your patch,
> here, would flatten each tuples as long as the table AMs don't 
> match.  That can be made cheaper in some cases.

I am confused by this. The toast-related table AM API functions are:
relation_needs_toast_table(), relation_toast_am(), and
relation_fetch_toast_slice().

What cases are we trying to solve here?

1. target of conversion is tableam1 main table, heap toast table
2. target of conversion is tableam1 main table, no toast table
3. target of conversion is tableam1 main table, tableam1 toast table
4. target of conversion is tableam1 main table, tableam2 toast table

Or does the problem apply to all of these cases?

And if tableam1 can't handle some case, why can't it just detoast the
data itself? Shouldn't that be able to decompress anything?

For example, in columnar[1], we just always detoast/decompress because
we want to compress it ourselves (along with other values from the same
column), and we never have a separate toast table. Is that code
incorrect, or will it break in v14?

Regards,
	Jeff Davis


https://github.com/citusdata/citus/blob/6b1904d37a18e2975b46f0955076f84c8a387cc6/src/backend/columnar/columnar_tableam.c#L1433

  8. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-06T20:23:05Z

    On Thu, May 06, 2021 at 01:10:53PM -0700, Jeff Davis wrote:
> On Mon, 2021-03-08 at 16:30 +0900, Michael Paquier wrote:
> > This toast issue is a kind of interesting one, and it seems rather
> > right to rely on toast_build_flattened_tuple() to decompress things
> > if
> > both table AMs support toast with the internals of toast knowing what
> > kind of compression has been applied to the stored tuple, rather than
> > have the table AM try to extra a toast tuple by itself.  I wonder
> > whether we should have a table AM API to know what kind of
> > compression
> > is supported for a given table AMs at hand, because there is no need
> > to flatten things if both the origin and the target match their
> > compression algos, which would be on HEAD to make sure that both the
> > origin and table AMs have toast (relation_toast_am).  Your patch,
> > here, would flatten each tuples as long as the table AMs don't 
> > match.  That can be made cheaper in some cases.
> 
> I am confused by this. The toast-related table AM API functions are:
> relation_needs_toast_table(), relation_toast_am(), and
> relation_fetch_toast_slice().

I wrote this shortly after looking at one of Dilip's LZ4 patches.

At one point in February/March, pg_attribute.attcompression defined the
compression used by *all* tuples in the table, rather than the compression used
for new tuples, and ALTER SET COMPRESSION would rewrite the table with the new
compression (rather than being only a catalog update).


> What cases are we trying to solve here?
> 
> 1. target of conversion is tableam1 main table, heap toast table
> 2. target of conversion is tableam1 main table, no toast table
> 3. target of conversion is tableam1 main table, tableam1 toast table
> 4. target of conversion is tableam1 main table, tableam2 toast table

I think ALTER TABLE SET ACCESS METHOD should move all data off the old AM,
including its toast table.  The optimization Michael suggests is when the new
AM and old AM use the same toast AM, then the data doesn't need to be
de/re/toasted.

Thanks for looking.

-- 
Justin

  9. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-05-06T21:11:31Z

    On Thu, 2021-05-06 at 15:23 -0500, Justin Pryzby wrote:
> I think ALTER TABLE SET ACCESS METHOD should move all data off the
> old AM,
> including its toast table.

Can you explain what you mean, and why? I'm still confused.

Let's say there are 4 table AMs: A, AT, B, and BT. A's
relation_toast_am() returns AT, and B's relation_toast_am() returns BT.
AT or BT are invalid if A or B have relation_needs_toast_table() return
false.

Here are the cases that I see:

If A = B, then AT = BT, and it's all a no-op.

If A != B and BT is invalid (e.g. converting heap to columnar), then A
should detoast (and perhaps decompress, as in the case of columnar)
whatever it gets as input and do whatever it wants. That's what
columnar does and I don't see why ATRewriteTable needs to handle it.

If A != B and AT != BT, then B needs to detoast whatever it gets (but
should not decompress, as that would just be wasted effort), and then
re-toast using BT. Again, I don't see a need for ATRewriteTable to do
anything, B can handle it.

The only case I can really see where ATRewriteTable might be helpful is
if A != B but AT = BT. In that case, in theory, you don't need to do
anything to the toast table, just leave it where it is. But then the
responsibilities get a little confusing to me -- how is B supposed to
know that it doesn't need to toast anything? Is this the problem you
are trying to solve?

Regards,
	Jeff Davis

  10. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-06T22:19:44Z

    On Thu, May 06, 2021 at 02:11:31PM -0700, Jeff Davis wrote:
> On Thu, 2021-05-06 at 15:23 -0500, Justin Pryzby wrote:
> > I think ALTER TABLE SET ACCESS METHOD should move all data off the
> > old AM,
> > including its toast table.
> 
> Can you explain what you mean, and why? I'm still confused.
> 
> Let's say there are 4 table AMs: A, AT, B, and BT. A's
> relation_toast_am() returns AT, and B's relation_toast_am() returns BT.
> AT or BT are invalid if A or B have relation_needs_toast_table() return
> false.
> 
> Here are the cases that I see:
> 
> If A = B, then AT = BT, and it's all a no-op.
> 
> If A != B and BT is invalid (e.g. converting heap to columnar), then A
> should detoast (and perhaps decompress, as in the case of columnar)
> whatever it gets as input and do whatever it wants. That's what
> columnar does and I don't see why ATRewriteTable needs to handle it.
> 
> If A != B and AT != BT, then B needs to detoast whatever it gets (but
> should not decompress, as that would just be wasted effort), and then
> re-toast using BT. Again, I don't see a need for ATRewriteTable to do
> anything, B can handle it.
> 
> The only case I can really see where ATRewriteTable might be helpful is
> if A != B but AT = BT. In that case, in theory, you don't need to do
> anything to the toast table, just leave it where it is. But then the
> responsibilities get a little confusing to me -- how is B supposed to
> know that it doesn't need to toast anything? Is this the problem you
> are trying to solve?

That's the optimization Michael is suggesting.

I was approaching this after having just looked at Dilip's patch (which was
originally written using pg_am to support "pluggable" compression "AM"s, but
not otherwise related to table AM).

Once a table is converted to a new AM, its tuples had better not reference the
old AM - it could be dropped.

The new table AM (B) shouldn't need to know anything about the old one (A).  It
should just process incoming tuples.  It makes more to me that ATRewriteTable
would handle this, rather than every acccess method having the same logic (at
best) or different logic (more likely).  If ATRewriteTable didn't handle this,
data would become inaccessible if an AM failed to de-toast tuples.

-- 
Justin

  11. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-05-07T00:24:25Z

    On Thu, 2021-05-06 at 17:19 -0500, Justin Pryzby wrote:
> If ATRewriteTable didn't
> handle this,
> data would become inaccessible if an AM failed to de-toast tuples.

If the AM fails to detoast tuples, it's got bigger problems than ALTER
TABLE. What about INSERT INTO ... SELECT?

It's the table AM's responsibility to detoast out-of-line datums and
toast any values that are too large (see
heapam.c:heap_prepare_insert()).

Regards,
	Jeff Davis

  12. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-06-03T21:36:15Z

    On Thu, 2021-05-06 at 17:24 -0700, Jeff Davis wrote:
> It's the table AM's responsibility to detoast out-of-line datums and
> toast any values that are too large (see
> heapam.c:heap_prepare_insert()).

Do we have general agreement on this point? Did I miss another purpose
of detoasting in tablecmds.c, or can we just remove that part of the
patch?

Regards,
	Jeff Davis

  13. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-06-04T05:58:13Z

    On Thu, Jun 03, 2021 at 02:36:15PM -0700, Jeff Davis wrote:
> Do we have general agreement on this point? Did I miss another purpose
> of detoasting in tablecmds.c, or can we just remove that part of the
> patch?

Catching up with this thread..  So, what you are suggesting here is
that we have no need to let ATRewriteTable() do anything about the
detoasting, and just push down the responsability of detoasting the
tuple, if necessary, down to the AM layer where the tuple insertion is
handled, right?

In short, a table AMs would receive on a rewrite with ALTER TABLE
tuples which may be toasted, still table_insert_tuple() should be able
to handle both:
- the case where this tuple was already toasted.
- the case where this tuple has been already detoasted.

You are right that this would be more consistent with what heap does
with heap_prepare_insert().
--
Michael

  14. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-06-04T18:26:28Z

    On Fri, 2021-06-04 at 14:58 +0900, Michael Paquier wrote:
> In short, a table AMs would receive on a rewrite with ALTER TABLE
> tuples which may be toasted, still table_insert_tuple() should be
> able
> to handle both:
> - the case where this tuple was already toasted.
> - the case where this tuple has been already detoasted.

Yes. That's a current requirement, and any AM that doesn't do that is
already broken (e.g. for INSERT INTO ... SELECT).

Regards,
	Jeff Davis

  15. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-06-04T23:45:46Z

    On Fri, Jun 04, 2021 at 11:26:28AM -0700, Jeff Davis wrote:
> Yes. That's a current requirement, and any AM that doesn't do that is
> already broken (e.g. for INSERT INTO ... SELECT).

Makes sense.  I was just looking at the patch, and this was the only
part of it that made my spidey sense react.

One thing I am wondering is if we should have a dummy_table_am in
src/test/modules/ to be able to stress more this feature.  That does
not seem like a hard requirement, but relying only on heap limits a
bit the coverage of this feature even if one changes
default_table_access_method.
--
Michael

  16. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-06-05T00:34:36Z

    On Sat, 2021-06-05 at 08:45 +0900, Michael Paquier wrote:
> One thing I am wondering is if we should have a dummy_table_am in
> src/test/modules/ to be able to stress more this feature.  That does
> not seem like a hard requirement, but relying only on heap limits a
> bit the coverage of this feature even if one changes
> default_table_access_method.

I agree that a dummy AM would be good, but implementing even a dummy AM
is a fair amount of work. Also, there are many potential variations, so
we'd probably need several.

The table AM API is a work in progress, and I think it will be a few
releases (and require a few more table AMs in the wild) to really nail
down the API.

Regards,
	Jeff Davis

  17. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-06-05T04:21:35Z

    On Fri, Jun 04, 2021 at 05:34:36PM -0700, Jeff Davis wrote:
> I agree that a dummy AM would be good, but implementing even a dummy AM
> is a fair amount of work.

Not much, honestly, the largest part being to document that properly
so as it could be used as a template:
https://www.postgresql.org/message-id/YEXm2nh/5j5P2jEl@paquier.xyz

> Also, there are many potential variations, so
> we'd probably need several.

Not so sure here.  GUCs or reloptions could be used to control some of
the behaviors.  Now this really depends on the use-cases we are
looking to support here and the low-level facilities that could
benefit from this module (dummy_index_am tests reloptions for
example).  I agree that this thread is perhaps not enough to justify
adding this module for now.

> The table AM API is a work in progress, and I think it will be a few
> releases (and require a few more table AMs in the wild) to really nail
> down the API.

Hard to say, we'll see.  I'd like to believe that it could be a good
to not set something in stone for that forever.
--
Michael

  18. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-06-09T00:33:31Z

    On Thu, 2021-06-03 at 14:36 -0700, Jeff Davis wrote:
> Do we have general agreement on this point? Did I miss another
> purpose
> of detoasting in tablecmds.c, or can we just remove that part of the
> patch?

Per discussion with Justin, I'll drive this patch. I merged the CF
entries into https://commitfest.postgresql.org/33/3110/

New version attached, with the detoasting code removed. Could use
another round of validation/cleanup in case I missed something during
the merge.

Regards,
	Jeff Davis

  19. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-06-09T04:47:18Z

    On Tue, Jun 08, 2021 at 05:33:31PM -0700, Jeff Davis wrote:
> New version attached, with the detoasting code removed. Could use
> another round of validation/cleanup in case I missed something during
> the merge.

This looks rather sane to me, thanks.

    /* Create the transient table that will receive the re-ordered data */
    OIDNewHeap = make_new_heap(tableOid, tableSpace,
+                              accessMethod
It strikes me that we could extend CLUSTER/VACUUM FULL to support this
option, in the same vein as TABLESPACE.  Perhaps that's not something to
implement or have, just wanted to mention it.

+ALTER TABLE heaptable SET ACCESS METHOD heap2;
+explain (analyze, costs off, summary off, timing off) SELECT * FROM heaptable;
+SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heaptable;
+DROP TABLE heaptable;
There is a mix of upper and lower-case characters here.  It could be
more consistent.  It seems to me that this test should actually check
that pg_class.relam reflects the new value.

+   /* Save info for Phase 3 to do the real work */
+   if (OidIsValid(tab->newAccessMethod))
+       ereport(ERROR,
+               (errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
Worth adding a test?

- * with the specified persistence, which might differ from the original's.
+ * NewTableSpace/accessMethod/persistence, which might differ from those
Nit: the name of the variable looks inconsistent with this comment.
The original is weird as well.

I am wondering if it would be a good idea to set the new tablespace
and new access method fields to InvalidOid within ATGetQueueEntry.  We
do that for the persistence.  Not critical at all, still..

+           pass = AT_PASS_MISC;
Maybe add a comment here?
--
Michael

  20. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-06-09T19:31:28Z

    On Wed, 2021-06-09 at 13:47 +0900, Michael Paquier wrote:
> There is a mix of upper and lower-case characters here.  It could be
> more consistent.  It seems to me that this test should actually check
> that pg_class.relam reflects the new value.

Done. I also added a (negative) test for changing the AM of a
partitioned table, which wasn't caught before.

> +                errmsg("cannot have multiple SET ACCESS METHOD
> subcommands")));
> Worth adding a test?

Done.

> Nit: the name of the variable looks inconsistent with this comment.
> The original is weird as well.

Tried to improve wording.

> I am wondering if it would be a good idea to set the new tablespace
> and new access method fields to InvalidOid within
> ATGetQueueEntry.  We
> do that for the persistence.  Not critical at all, still..

Done.

> +           pass = AT_PASS_MISC;
> Maybe add a comment here?

Done. In that case, it doesn't matter because there's no work to be
done in Phase 2.

Regards,
	Jeff Davis

  21. Re: alter table set TABLE ACCESS METHOD

    Zhihong Yu <zyu@yugabyte.com> — 2021-06-09T20:45:52Z

    On Wed, Jun 9, 2021 at 12:31 PM Jeff Davis <pgsql@j-davis.com> wrote:

> On Wed, 2021-06-09 at 13:47 +0900, Michael Paquier wrote:
> > There is a mix of upper and lower-case characters here.  It could be
> > more consistent.  It seems to me that this test should actually check
> > that pg_class.relam reflects the new value.
>
> Done. I also added a (negative) test for changing the AM of a
> partitioned table, which wasn't caught before.
>
> > +                errmsg("cannot have multiple SET ACCESS METHOD
> > subcommands")));
> > Worth adding a test?
>
> Done.
>
> > Nit: the name of the variable looks inconsistent with this comment.
> > The original is weird as well.
>
> Tried to improve wording.
>
> > I am wondering if it would be a good idea to set the new tablespace
> > and new access method fields to InvalidOid within
> > ATGetQueueEntry.  We
> > do that for the persistence.  Not critical at all, still..
>
> Done.
>
> > +           pass = AT_PASS_MISC;
> > Maybe add a comment here?
>
> Done. In that case, it doesn't matter because there's no work to be
> done in Phase 2.
>
> Regards,
>         Jeff Davis
>
> Hi,

+           /* check if another access method change was already requested
*/
+           if (tab->newAccessMethod)
+               ereport(ERROR,
+                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                        errmsg("cannot change access method setting
twice")));

I think the error message can be refined - changing  access method twice is
supported, as long as the two changes don't overlap.

Cheers

  22. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-06-10T02:02:04Z

    On Wed, Jun 09, 2021 at 01:45:52PM -0700, Zhihong Yu wrote:
> +           /* check if another access method change was already requested
> */
> +           if (tab->newAccessMethod)
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                        errmsg("cannot change access method setting
> twice")));
> 
> I think the error message can be refined - changing  access method twice is
> supported, as long as the two changes don't overlap.

Hmm.  Do we actually need this one?  ATPrepSetAccessMethod() checks
already that this command cannot be specified multiple times, with an
error message consistent with what SET TABLESPACE does.
--
Michael

  23. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-10T02:35:06Z

    On Wed, Jun 09, 2021 at 01:47:18PM +0900, Michael Paquier wrote:
> On Tue, Jun 08, 2021 at 05:33:31PM -0700, Jeff Davis wrote:
> > New version attached, with the detoasting code removed. Could use
> > another round of validation/cleanup in case I missed something during
> > the merge.
> 
> This looks rather sane to me, thanks.
> 
>     /* Create the transient table that will receive the re-ordered data */
>     OIDNewHeap = make_new_heap(tableOid, tableSpace,
> +                              accessMethod
> It strikes me that we could extend CLUSTER/VACUUM FULL to support this
> option, in the same vein as TABLESPACE.  Perhaps that's not something to
> implement or have, just wanted to mention it.

It's a good thought.  But remember that that c5b28604 handled REINDEX
(TABLESPACE) but not CLUSTER/VACUUM FULL (TABLESPACE).  You wrote:
https://www.postgresql.org/message-id/YBuWbzoW6W7AaMv0%40paquier.xyz
> Regarding the VACUUM and CLUSTER cases, I am not completely sure if
> going through these for a tablespace case is the best move we can do,
> as ALTER TABLE is able to mix multiple operations and all of them
> require already an AEL to work.  REINDEX was different thanks to the
> case of CONCURRENTLY.  Anyway, as a lot of work has been done here
> already, I would recommend to create new threads about those two
> topics.  I am also closing this patch in the CF app.

In any case, I think we really want to have an ALTER .. SET ACCESS METHOD.
Supporting it also in CLUSTER/VACUUM is an optional, additional feature.

-- 
Justin

  24. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-10T02:40:14Z

    On Wed, Jun 09, 2021 at 01:45:52PM -0700, Zhihong Yu wrote:
> +           /* check if another access method change was already requested
> */
> +           if (tab->newAccessMethod)
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                        errmsg("cannot change access method setting twice")));
> 
> I think the error message can be refined - changing  access method twice is
> supported, as long as the two changes don't overlap.

That language is consistent wtih existing errors.

src/backend/commands/tablecmds.c:                                                errmsg("cannot change persistence setting twice")));
src/backend/commands/tablecmds.c:                                                errmsg("cannot change persistence setting twice")));
                                 errmsg("cannot alter type of column \"%s\" twice",

However, I think that SET TABLESPACE is a better template to follow:
                                 errmsg("cannot have multiple SET TABLESPACE subcommands")));

Michael pointed out that there's two, redundant checks:

+       if (rel->rd_rel->relam == amoid)
+               return;
+  
+       /* Save info for Phase 3 to do the real work */
+       if (OidIsValid(tab->newAccessMethod))
+               ereport(ERROR,
+                               (errcode(ERRCODE_SYNTAX_ERROR),
+                                errmsg("cannot have multiple SET ACCESS METHOD subcommands")));

I think that the "multiple subcommands" test should be before the "no-op" test.

@Jeff: In my original patch, I also proposed patches 2,3:

Subject: [PATCH v2 2/3] Allow specifying acccess method of partitioned tables..
Subject: [PATCH v2 3/3] Implement lsyscache get_rel_relam()

  25. Re: alter table set TABLE ACCESS METHOD

    vignesh C <vignesh21@gmail.com> — 2021-07-14T11:01:45Z

    On Thu, Jun 10, 2021 at 1:01 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Wed, 2021-06-09 at 13:47 +0900, Michael Paquier wrote:
> > There is a mix of upper and lower-case characters here.  It could be
> > more consistent.  It seems to me that this test should actually check
> > that pg_class.relam reflects the new value.
>
> Done. I also added a (negative) test for changing the AM of a
> partitioned table, which wasn't caught before.
>
> > +                errmsg("cannot have multiple SET ACCESS METHOD
> > subcommands")));
> > Worth adding a test?
>
> Done.
>
> > Nit: the name of the variable looks inconsistent with this comment.
> > The original is weird as well.
>
> Tried to improve wording.
>
> > I am wondering if it would be a good idea to set the new tablespace
> > and new access method fields to InvalidOid within
> > ATGetQueueEntry.  We
> > do that for the persistence.  Not critical at all, still..
>
> Done.
>
> > +           pass = AT_PASS_MISC;
> > Maybe add a comment here?
>
> Done. In that case, it doesn't matter because there's no work to be
> done in Phase 2.
>

There are few compilation issues:
tablecmds.c:4629:52: error: too few arguments to function call,
expected 3, have 2
ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW);
~~~~~~~~~~~~~~~~~~~ ^
tablecmds.c:402:1: note: 'ATSimplePermissions' declared here
static void ATSimplePermissions(AlterTableType cmdtype, Relation rel,
int allowed_targets);
^
tablecmds.c:5983:10: warning: enumeration value 'AT_SetAccessMethod'
not handled in switch [-Wswitch]
switch (cmdtype)
^
1 warning and 1 error generated.

Also few comments need to be addressed, based on that I'm changing the
status to "Waiting for Author".

Regards,
Vignesh

  26. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-07-16T03:49:23Z

    rebased.

Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
But one of them wasn't hit if the ALTER was setting the current AM due to the
no-op test.

I think it's better to fail in every case, and not just sometimes (especially
if we were to use ERRCODE_SYNTAX_ERROR).

I included my 2ndary patch allowing to set the AM of partitioned table, same as
for a tablespace.

-- 
Justin

  27. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-07-19T07:21:50Z

    On Thu, Jul 15, 2021 at 10:49:23PM -0500, Justin Pryzby wrote:
> Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
> But one of them wasn't hit if the ALTER was setting the current AM due to the
> no-op test.

Yep.

> I think it's better to fail in every case, and not just sometimes (especially
> if we were to use ERRCODE_SYNTAX_ERROR).

Looks rather fine.

-           if (tab->newTableSpace)
+           if (OidIsValid(tab->newTableSpace))
This has no need to be part of this patch.

    /*
-    * If we have ALTER TABLE <sth> SET TABLESPACE provide a list of
-    * tablespaces
+    * Complete with list of tablespaces (for SET TABLESPACE) or table AMs (for
+    * SET ACCESS METHOD).
     */
+   else if (Matches("ALTER", "TABLE", MatchAny, "SET", "ACCESS", "METHOD"))
+       COMPLETE_WITH_QUERY(Query_for_list_of_table_access_methods);
    else if (Matches("ALTER", "TABLE", MatchAny, "SET", "TABLESPACE"))
        COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
Nit, there is no need to merge both block here.  Let's keep them
separated.

+-- negative test
[...]
+-- negative test
Those descriptions could be better, and describe what they prevent
(aka no multiple subcommands SET ACCESS METHOD and not allowed on
partitioned tables).

> I included my 2ndary patch allowing to set the AM of partitioned table, same as
> for a tablespace.

I would suggest to not hide this topic within a thread unrelated to
it, as this is not going to ease the feedback around it.  Let's start
a new thread if you feel this is necessary.

Jeff, you proposed to commit this patch upthread.  Are you planning to
look at that and do so?
--
Michael

  28. Re: alter table set TABLE ACCESS METHOD

    vignesh C <vignesh21@gmail.com> — 2021-07-22T05:07:12Z

    On Fri, Jul 16, 2021 at 9:19 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> rebased.
>
> Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
> But one of them wasn't hit if the ALTER was setting the current AM due to the
> no-op test.
>
> I think it's better to fail in every case, and not just sometimes (especially
> if we were to use ERRCODE_SYNTAX_ERROR).
>
> I included my 2ndary patch allowing to set the AM of partitioned table, same as
> for a tablespace.

One of the tests is failing, please post an updated patch for this:
create_am.out       2021-07-22 10:34:56.234654166 +0530
@@ -177,6 +177,7 @@
 (1 row)

 -- CREATE TABLE ..  PARTITION BY supports USING
+-- new partitions will inherit from the current default, rather the
partition root
 CREATE TABLE tableam_parted_heap2 (a text, b int) PARTITION BY list
(a) USING heap2;
 SET default_table_access_method = 'heap';
 CREATE TABLE tableam_parted_a_heap2 PARTITION OF tableam_parted_heap2
FOR VALUES IN ('a');

Regards,
Vignesh

  29. Re: alter table set TABLE ACCESS METHOD

    Justin Pryzby <pryzby@telsasoft.com> — 2021-07-22T09:41:54Z

    On Thu, Jul 22, 2021 at 10:37:12AM +0530, vignesh C wrote:
> On Fri, Jul 16, 2021 at 9:19 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > rebased.
> >
> > Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
> > But one of them wasn't hit if the ALTER was setting the current AM due to the
> > no-op test.
> >
> > I think it's better to fail in every case, and not just sometimes (especially
> > if we were to use ERRCODE_SYNTAX_ERROR).
> >
> > I included my 2ndary patch allowing to set the AM of partitioned table, same as
> > for a tablespace.
> 
> One of the tests is failing, please post an updated patch for this:
> create_am.out       2021-07-22 10:34:56.234654166 +0530

It looks like one hunk was missing/uncommitted from the 0002 patch..

-- 
Justin

  30. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-07-27T07:38:48Z

    On Thu, Jul 22, 2021 at 04:41:54AM -0500, Justin Pryzby wrote:
> It looks like one hunk was missing/uncommitted from the 0002 patch..

Okay, hearing nothing, I have looked again at 0001 and did some light
adjustments, mainly in the tests.  I did not spot any issues in the
patch, so that looks good to go for me.
--
Michael

  31. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-07-28T03:23:44Z

    On Tue, Jul 27, 2021 at 04:38:48PM +0900, Michael Paquier wrote:
> Okay, hearing nothing, I have looked again at 0001 and did some light
> adjustments, mainly in the tests.  I did not spot any issues in the
> patch, so that looks good to go for me.

And done as of b048326.
--
Michael

  32. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-07-28T03:40:59Z

    On Wed, 2021-07-28 at 12:23 +0900, Michael Paquier wrote:
> On Tue, Jul 27, 2021 at 04:38:48PM +0900, Michael Paquier wrote:
> > Okay, hearing nothing, I have looked again at 0001 and did some
> > light
> > adjustments, mainly in the tests.  I did not spot any issues in the
> > patch, so that looks good to go for me.
> 
> And done as of b048326.

I just returned from vacation and I was about ready to commit this
myself, but I noticed that it doesn't seem to be calling
InvokeObjectPostAlterHook(). I was in the process of trying to be sure
of where to call it. It looks like it should be called after catalog
changes but before CommandCounterIncrement(), and it also looks like it
should be called even for no-op commands.

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
TABLESPACE.

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?

Regards,
	Jeff Davis

  33. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-07-28T05:02:41Z

    On Tue, Jul 27, 2021 at 08:40:59PM -0700, Jeff Davis wrote:
> I just returned from vacation and I was about ready to commit this
> myself, but I noticed that it doesn't seem to be calling
> InvokeObjectPostAlterHook().

Arg, sorry about that!  I was unclear what the situation of the patch
was.

> I was in the process of trying to be sure
> of where to call it. It looks like it should be called after catalog
> changes but before CommandCounterIncrement(), and it also looks like it
> should be called even for no-op commands.

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.

> 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
> TABLESPACE.

Hmm.  Okay.

> 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.
--
Michael

  34. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-07-28T20:05:10Z

    On Wed, 2021-07-28 at 14:02 +0900, Michael Paquier wrote:
> Arg, sorry about that!  I was unclear what the situation of the patch
> was.

No problem, race condition ;-)

> 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.

> > 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
> > TABLESPACE.
> 
> Hmm.  Okay.
> 
> > 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.

Regards,
	Jeff Davis

  35. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-07-29T06:27:35Z

    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
METHOD :(

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
>>> TABLESPACE.
>> 
>> 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
complication.

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.
--
Michael

  36. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-07-29T15:55:21Z

    On Thu, 2021-07-29 at 15:27 +0900, Michael Paquier wrote:
> 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
> METHOD :(
> 
> 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.

I see that ATExecSetTableSpace() also invokes the hook even for a no-
op. Should we do the same thing for setting the AM?

> > > > 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
> > > > TABLESPACE.
> > > 
> > > Hmm.  Okay.
> 
> I'd still disagree with that.

OK, I won't press for a change here.

Regards,
	Jeff Davis

  37. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-07-30T07:22:19Z

    On Thu, Jul 29, 2021 at 08:55:21AM -0700, Jeff Davis wrote:
> I see that ATExecSetTableSpace() also invokes the hook even for a no-
> op. Should we do the same thing for setting the AM?

Looking at the past, it was the intention of 05f3f9c7 to go through
the hook even if SET TABLESPACE does not move the relation, so you are
right that ALTER TABLE is inconsistent to not do the same for LOGGED,
UNLOGGED and ACCESS METHOD if all of them do nothing to trigger a
relation rewrite.

Now, I am a bit biased about this change and if we actually need it
for the no-op path.  If we were to do that, I think that we need to
add in AlteredTableInfo a way to track down if any of those
subcommands have been used to allow the case of rewrite == 0 to launch
the hook even if these are no-ops.  And I am not sure if that's worth
the code complication for an edge case.  We definitely should have a
hook call for the case of rewrite > 0, though.
--
Michael

  38. Re: alter table set TABLE ACCESS METHOD

    Jeff Davis <pgsql@j-davis.com> — 2021-07-30T21:18:02Z

    On Fri, 2021-07-30 at 16:22 +0900, Michael Paquier wrote:
> Looking at the past, it was the intention of 05f3f9c7 to go through
> the hook even if SET TABLESPACE does not move the relation, so you
> are
> right that ALTER TABLE is inconsistent to not do the same for LOGGED,
> UNLOGGED and ACCESS METHOD if all of them do nothing to trigger a
> relation rewrite.
> 
> Now, I am a bit biased about this change and if we actually need it
> for the no-op path.  If we were to do that, I think that we need to
> add in AlteredTableInfo a way to track down if any of those
> subcommands have been used to allow the case of rewrite == 0 to
> launch
> the hook even if these are no-ops.  And I am not sure if that's worth
> the code complication for an edge case.  We definitely should have a
> hook call for the case of rewrite > 0, though.

It sounds like anything we do here should be part of a larger change to
make it consistent. So I'm fine with the patch you posted.

Regards,
	Jeff Davis

  39. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-08-07T10:18:19Z

    On Fri, Jul 30, 2021 at 02:18:02PM -0700, Jeff Davis wrote:
> It sounds like anything we do here should be part of a larger change to
> make it consistent. So I'm fine with the patch you posted.

As a matter of curiosity, I have checked how it would look to handle
the no-op case for the sub-commands other than SET TABLESPACE, and one
would need something like the attached, with a new flag for
AlteredTableInfo.  That does not really look good, but it triggers
properly the object access hook when SET LOGGED/UNLOGGED/ACCESS METHOD
are no-ops, so let's just handle the case using the version from
upthread.  I'll do that at the beginning of next week.
--
Michael

  40. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-08-10T03:24:13Z

    On Sat, Aug 07, 2021 at 07:18:19PM +0900, Michael Paquier wrote:
> As a matter of curiosity, I have checked how it would look to handle
> the no-op case for the sub-commands other than SET TABLESPACE, and one
> would need something like the attached, with a new flag for
> AlteredTableInfo.  That does not really look good, but it triggers
> properly the object access hook when SET LOGGED/UNLOGGED/ACCESS METHOD
> are no-ops, so let's just handle the case using the version from
> upthread.  I'll do that at the beginning of next week.

So, on a closer look, it happens that this breaks the regression tests
of sepgsql, as the two following commands in ddl.sql cause a rewrite:
ALTER TABLE regtest_table_4 ALTER COLUMN y TYPE float;
ALTER TABLE regtest_ptable_4 ALTER COLUMN y TYPE float;

I have been fighting with SE/Linux for a couple of hours to try to
figure out how to run our regression tests, but gave up after running
into various segmentation faults even after following all the steps of
the documentation to set up things.  Perhaps that's because I just set
up the environment with a recent Debian, I don't know.  Instead of
running those tests, I have fallen back to my own module and ran all
the SQLs of sepgsql to find out places where there are rewrites where
I spotted those two places.

One thing I have noticed is that sepgsql-regtest.te fails to compile
because /usr/share/selinux/devel/Makefile does not understand
auth_read_passwd().  Looking at some of the SE/Linux repos, perhaps
this ought to be auth_read_shadow() instead to be able to work with a
newer version?

Anyway, as the addition of this InvokeObjectPostAlterHook() is
consistent for a rewrite caused by SET LOGGED/UNLOGGED/ACCESS METHOD I
have applied the patch.  I'll fix rhinoceros once it reports back the
diffs in output.
--
Michael

  41. Re: alter table set TABLE ACCESS METHOD

    Michael Paquier <michael@paquier.xyz> — 2021-08-10T04:17:52Z

    On Tue, Aug 10, 2021 at 12:24:13PM +0900, Michael Paquier wrote:
> So, on a closer look, it happens that this breaks the regression tests
> of sepgsql, as the two following commands in ddl.sql cause a rewrite:
> ALTER TABLE regtest_table_4 ALTER COLUMN y TYPE float;
> ALTER TABLE regtest_ptable_4 ALTER COLUMN y TYPE float;

rhinoceros has reported back, and these are the only two that required
an adjustment, so fixed.
--
Michael