Re: preserving forensic information when we freeze

On Fri, Dec 27, 2013 at 9:24 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> I'm not sure what you mean by "doesn't work", because it clearly does
>> work. I've already posted a patch. You may find it ugly, but that's
>> not the same as not working.
>
> I meant *practically*, it doesn't work. By which, I mean, "it sucks" as
> a solution. :)

I fail to see why. What is so ugly about this:

select x.* from pgbench_accounts a, pg_tuple_header(a.tableoid, a.ctid) x;

It may be true that that query wouldn't have worked before Tom added
LATERAL, but he did add LATERAL and we have it now and nobody's going
to care about this function on versions that haven't got LATERAL,
because it won't exist there anyway. The whole point of adding
features like LATERAL (which doesn't even appear in that query,
interestingly enough) is that it was really annoying to use functions
like this before we had it. But now we *do* have it, so the fact a
function like this would have been annoying to use in older releases
isn't a reason not to add it now.

I will admit that performance may be a reason. After pgbench -i:

rhaas=# explain analyze select xmax from pgbench_accounts;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
Seq Scan on pgbench_accounts (cost=0.00..2640.00 rows=100000
width=4) (actual time=0.009..14.950 rows=100000 loops=1)
Total runtime: 20.354 ms
(2 rows)

rhaas=# explain analyze select (pg_tuple_header(tableoid,ctid)).xmax
from pgbench_accounts;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------
Seq Scan on pgbench_accounts (cost=0.00..2890.00 rows=100000
width=10) (actual time=0.023..314.783 rows=100000 loops=1)
Total runtime: 322.714 ms
(2 rows)

>> > Hindsight being what it is, perhaps we should have stuffed the system
>> > columns into a complex type instead of having individual columns, but
>> > I'm not sure changing that now would be worth the backwards
>> > compatibility break (yes, I know they're system columns, but I've seen
>> > more than one case of using ctid to break ties in otherwise identical
>> > rows..).
>>
>> Well, if the consensus is in favor of adding more system columns,
>> that's not my first choice, but I'm OK with it. However, I wonder how
>> we plan to name them. If we add pg_infomask and pg_infomask2, it
>> won't be consistent with the existing naming convention which doesn't
>> include any kind of pg-prefix, but if we don't use such a prefix then
>> every column we add further pollutes the namespace.
>
> Yeah, I agree that it gets a bit ugly... What would you think of doing
> *both*? Keep the existing system columns for backwards compatibility,
> but then also have a complex 'pg_header' type which provides all of the
> existing columns, as well as infomask && infomask2 ...?

I don't really see what that accomplishes, TBH. If we further modify
the header format in the future, we'll need to modify the definition
of that type, and that will be a backward-compatibility break for
anyone using it. Adding new system columns is probably less
intrusive.

Maybe it's best to just add pg_infomask and pg_infomask2 as system
columns and not worry about the inconsistency with the naming
convention for existing columns.

Other opinions?

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