Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED

From: Christoph Berg <cb(at)df7cb(dot)de>
To: Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)com>
Cc: Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>
Subject: Re: [GSoC2014] Patch ALTER TABLE ... SET LOGGED
Date: 2014-07-14 18:31:34
Message-ID: 20140714183134.GC14198@msg.df7cb.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Re: Fabrízio de Royes Mello 2014-07-12 <CAFcNs+qF5fUkkp9vdJWokiaBn_rRM4+HJqXeeVpD_7-tO0L4AA(at)mail(dot)gmail(dot)com>
> > ... that being the non-WAL-logging with wal_level=minimal, or more?
>
> This is the first of additional goals, but we have others. Please see [1].

Oh I wasn't aware of the wiki page, I had just read the old thread.
Thanks for the pointer.

> > > diff --git a/doc/src/sgml/ref/alter_table.sgml
> b/doc/src/sgml/ref/alter_table.sgml
> > > index 69a1e14..424f2e9 100644
> > > --- a/doc/src/sgml/ref/alter_table.sgml
> > > +++ b/doc/src/sgml/ref/alter_table.sgml
> > > @@ -58,6 +58,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable
> class="PARAMETER">name</replaceable>
> > > ENABLE REPLICA RULE <replaceable
> class="PARAMETER">rewrite_rule_name</replaceable>
> > > ENABLE ALWAYS RULE <replaceable
> class="PARAMETER">rewrite_rule_name</replaceable>
> > > CLUSTER ON <replaceable class="PARAMETER">index_name</replaceable>
> > > + SET {LOGGED | UNLOGGED}
> > > SET WITHOUT CLUSTER
> > > SET WITH OIDS
> > > SET WITHOUT OIDS
> >
> > This must not be between the two CLUSTER lines. I think the best spot
> > would just be one line down, before SET WITH OIDS.
>
> Fixed.

The (long) SET LOGGED paragraph is still between CLUSTER and SET
WITHOUT CLUSTER.

> > This grammar bug pops up consistently: This form *changes*...
> >
>
> Fixed.

Two more:

+ * The AlterTableChangeCatalogToLoggedOrUnlogged function perform the
+ * The AlterTableChangeIndexesToLoggedOrUnlogged function scan all indexes

> > This unnecessarily rewrites all the tabs, but see below.
> >
>
> I did that because the new constant AT_PASS_SET_LOGGED_UNLOGGED is larger
> than others.

Ah, ok then.

> > I'm wondering if you shouldn't make a single ATPrepSetLogged function
> > that takes and additional toLogged argument. Or alternatively get rid
> > of the if() by putting the code also into case AT_SetLogged.
> >
>
> Actually I started that way... with just one ATPrep function we have some
> additional complexity to check relpersistence, define the error message and
> to run AlterTableSetLoggedCheckForeignConstraints(rel) function. So to
> simplify the code I decided split in two small functions.

Nod.

> > > relation_close(rel, NoLock);
> > > +
> > > + if (pass == AT_PASS_SET_LOGGED_UNLOGGED)
> > > +
> ATPostAlterSetLoggedUnlogged(RelationGetRelid(rel));
> >
> > This must be done before relation_close() which releases all locks.

You didn't address that. I'm not sure about it, but either way, this
deserves a comment on the lock level necessary.

> > Moreover, I think you can get rid of that extra PASS here.
> > AT_PASS_ALTER_TYPE has its own pass because you can alter several
> > columns in a single ALTER TABLE statement, but you can have only one
> > SET (UN)LOGGED, so you can to the cluster_rel() directly in
> > AlterTableSetLoggedOrUnlogged() (unless cluster_rel() is too intrusive
> > and would interfere with other ALTER TABLE operations in this command,
> > no idea).
> >
>
> I had some troubles here so I decided to do in that way, but I confess I'm
> not comfortable with this implementation. Looking more carefully on
> tablecmds.c code, at the ATController we have three phases and the third is
> 'scan/rewrite tables as needed' so my doubt is if can I use it instead of
> call 'cluster_rel'?

I've just tried some SET (UN)LOGGED operations with altering column
types in the same operation, that works. But:

Yes, you should use the existing table rewriting machinery, or at
least clearly document (in comments) why it doesn't work for you.

Also looking at ATController, there's a wqueue mechanism to queue
catalog updates. You should probably use this, too, or again document
why it doesn't work for you.

> > Here's the big gotcha: Just like SET LOGGED must check for outgoing
> > FKs to unlogged tables, SET UNLOGGED must check for incoming FKs from
> > permanent tables. This is missing.
> >
>
> I don't think so... we can create an unlogged table with a FK referring to
> a regular table...
> ... but is not possible create a FK from a regular table referring to an
> unlogged table:
> ... and a FK from an unlogged table referring other unlogged table works:
> So we must take carefull just when changing an unlogged table to a regular
> table.
>
> Am I correct or I miss something?

You miss the symmetric case the other way round. When changing a table
to unlogged, you need to make sure no other permanent table is
referencing our table.

> > > +AlterTableChangeCatalogToLoggedOrUnlogged(Relation rel, Relation
> relrelation, bool toLogged)

You are using "relrelation" and "relrel". I'd change all occurrences
to "relrelation" because that's also used elsewhere.

> > The comment on heap_open() suggests that you could directly invoke
> > relation_open() because you know this is a toast table, similarly for
> > index_open(). (I can't say which is better style.)
> >
>
> I don't know which is better style too... other opinions??

Both are used several times in tablecmds.c, so both are probably fine.
(Didn't check the contexts, though.)

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2014-07-14 18:55:25 Re: Pg_upgrade and toast tables bug discovered
Previous Message Peter Geoghegan 2014-07-14 18:17:09 Re: B-Tree support function number 3 (strxfrm() optimization)