On Wed, Jun 9, 2021 at 12:31 PM Jeff Davis <[email protected]> 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
