Re: Fabrízio de Royes Mello 2014-07-12 <cafcns+qf5fukkp9vdjwokiabn_rrm4+hjqxeevpd_7-to0l...@mail.gmail.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
--
c...@df7cb.de | http://www.df7cb.de/
signature.asc
Description: Digital signature
