Re: logical changeset generation v6.2

On Mon, Oct 21, 2013 at 2:27 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I think it's a complete no-go. Consider, e.g., the comment for
>> MaxTupleAttributeNumber, which you've blithely falsified. Even if you
>> update the comment and the value, I'm not inspired by the idea of
>> subtracting 32 from that number; even if it weren't already too small,
>> it would break pg_upgrade for any users who are on the edge.
>
> Well, we only need to support it for (user_)catalog tables. So
> pg_upgrade isn't a problem. And I don't really see a problem restricting
> the number of columns for such tables.

Inch by inch, this patch set is trying to make catalog tables more and
more different from regular tables. I think that's a direction we're
going to regret. I can almost believe that no great harm will come to
us from giving the two different xmin horizons, as previously
discussed, though I'm still not 100% happy about that. Can't both
have something be a catalog table AND replicate it? Ick, but OK.

But changing the on disk format strikes me as crossing some sort of
bright line. That means that you're going to have two different code
paths in a lot of important cases, one for catalog tables and one for
non-catalog tables, and the one that's only taken for catalog tables
will be rather lightly traveled. And then you've got user catalog
tables, and the possibility that something that wasn't formerly a user
catalog table might become one, or visca versa. Even if you can flush
out every bug that exists today, this is a recipe for future bugs.

>> Things
>> aren't looking too good for anything that uses HeapTupleFields,
>> either; consider rewrite_heap_tuple().
>
> Well, that currently works, by copying cmax. Since rewriting triggered
> the change, I am pretty sure I've actually tested & hit that path...

No offense, but that's a laughable statement. If that path works,
it's mostly if not entirely by accident. You've fundamentally changed
the heap tuple format, and that code doesn't know about it, even
though it's deeply in bed with assumptions about the old format. I
think this is a pretty clear indication as to what's wrong with this
approach: a lot of stuff will not care, but the stuff that does care
will be hard to find, and future incremental modifications either to
that code or to the hidden data before the t_hoff pointer could break
stuff that formerly worked. We rejected early drafts of sepgsql RLS
cold because they changed the tuple format, and I don't see why we
shouldn't do exactly the same thing here.

But just suppose for a minute that we'd accepted that proposal and
then took this one, too. And let's suppose we also accept the next
proposal that, like that one and this one, jams something more into
the heap tuple header. At that point you'll have potentially as many
as 8 different maximum-number-of-attributes values for tuples, though
maybe not quite that many in practice if not all of those features can
be used together. The macros that are needed to extract the various
values from the heap tuple will be nightmarishly complex, and we'll
have eaten up all (or more than all) of our remaining bit-space in the
infomask. Maybe all of that sounds all right to you, but to me it
sounds like a big mess.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company