Re: Refactoring log_newpage

Lists: pgsql-hackers
From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Refactoring log_newpage
Date: 2012-02-01 10:25:58
Message-ID: CA+U5nM+zRO2WAMq_gLbhGompMJNs-p5bJrJh8LgaMD-VDv2Utg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

At present log_newpage() produces log records called XLOG_HEAP_NEWPAGE.

That routine is used by HEAP, BTREE, GIN, SPGIST rmgrs, as well as
various forks.

WAL contains no information as to which rmgr the data refers to,
making debugging much harder and skewing efforts to optimise WAL
traffic and is a pretty gross modularity violation of the whole rmgr
concept.

This refactoring adds an RmgrId field onto each new page record and
makes clearer that certain "heap" routines are actually generic. The
WAL records are still marked as HEAP rmgr and have XLOG_NEWPAGE record
type, but at least we can tell them apart. (We already had forknum,
just not rmgrid).

Another refactoring option would be to have specific record types for
each rmgr, and would normally be my preferred option but that seems
likely to use up too many record type numbers in the index rmgrs.

For immediate commit.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment Content-Type Size
refactor_newpage.v1.patch text/x-diff 10.5 KB

From: Jim Nasby <jim(at)nasby(dot)net>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring log_newpage
Date: 2012-02-01 22:42:27
Message-ID: 4FF85E04-6418-4A84-8541-9A868ABA8D75@nasby.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Feb 1, 2012, at 4:25 AM, Simon Riggs wrote:
> At present log_newpage() produces log records called XLOG_HEAP_NEWPAGE.
>
> That routine is used by HEAP, BTREE, GIN, SPGIST rmgrs, as well as
> various forks.
>
> WAL contains no information as to which rmgr the data refers to,
> making debugging much harder and skewing efforts to optimise WAL
> traffic and is a pretty gross modularity violation of the whole rmgr
> concept.
>
> This refactoring adds an RmgrId field onto each new page record and
> makes clearer that certain "heap" routines are actually generic. The
> WAL records are still marked as HEAP rmgr and have XLOG_NEWPAGE record
> type, but at least we can tell them apart. (We already had forknum,
> just not rmgrid).

But we already had RelFileNode; wouldn't that be enough to tell what rmgr was responsible for the new page? Can 2 different rmgrs write to the same file node?
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Jim Nasby <jim(at)nasby(dot)net>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring log_newpage
Date: 2012-02-02 06:19:23
Message-ID: CA+U5nM+k+i40zny-nX5LfNGk-pJcmZv5QQF613A=PRTVqANGGw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Wed, Feb 1, 2012 at 10:42 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
> On Feb 1, 2012, at 4:25 AM, Simon Riggs wrote:
>> At present log_newpage() produces log records called XLOG_HEAP_NEWPAGE.
>>
>> That routine is used by HEAP, BTREE, GIN, SPGIST rmgrs, as well as
>> various forks.
>>
>> WAL contains no information as to which rmgr the data refers to,
>> making debugging much harder and skewing efforts to optimise WAL
>> traffic and is a pretty gross modularity violation of the whole rmgr
>> concept.
>>
>> This refactoring adds an RmgrId field onto each new page record and
>> makes clearer that certain "heap" routines are actually generic. The
>> WAL records are still marked as HEAP rmgr and have XLOG_NEWPAGE record
>> type, but at least we can tell them apart. (We already had forknum,
>> just not rmgrid).
>
>
> But we already had RelFileNode; wouldn't that be enough to tell what rmgr was responsible for the new page? Can 2 different rmgrs write to the same file node?

No, but which one? No way to tell unless you have full list of
relfilenodes and check each one. So btree changes look like heap
changes etc..

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Jim Nasby <jim(at)nasby(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring log_newpage
Date: 2012-02-02 07:26:09
Message-ID: 4F2A3A91.4070001@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 02.02.2012 08:19, Simon Riggs wrote:
> On Wed, Feb 1, 2012 at 10:42 PM, Jim Nasby<jim(at)nasby(dot)net> wrote:
>> On Feb 1, 2012, at 4:25 AM, Simon Riggs wrote:
>>> At present log_newpage() produces log records called XLOG_HEAP_NEWPAGE.
>>>
>>> That routine is used by HEAP, BTREE, GIN, SPGIST rmgrs, as well as
>>> various forks.
>>>
>>> WAL contains no information as to which rmgr the data refers to,
>>> making debugging much harder and skewing efforts to optimise WAL
>>> traffic and is a pretty gross modularity violation of the whole rmgr
>>> concept.
>>>
>>> This refactoring adds an RmgrId field onto each new page record and
>>> makes clearer that certain "heap" routines are actually generic. The
>>> WAL records are still marked as HEAP rmgr and have XLOG_NEWPAGE record
>>> type, but at least we can tell them apart. (We already had forknum,
>>> just not rmgrid).
>>
>>
>> But we already had RelFileNode; wouldn't that be enough to tell what rmgr was responsible for the new page? Can 2 different rmgrs write to the same file node?
>
> No, but which one? No way to tell unless you have full list of
> relfilenodes and check each one.

Well, you can obviously check the catalogs for that, but you must be
assuming that you don't have access to the catalogs or this would be a
non-issue.

You can also identify the kind of page by looking at the special area of
the stored page. See:
http://archives.postgresql.org/pgsql-hackers/2007-04/msg00392.php

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: Jim Nasby <jim(at)nasby(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring log_newpage
Date: 2012-02-02 09:35:44
Message-ID: CA+U5nMJXw_iTubW=LQyS6roQOUmhVvv2B6zDGVWZsXDnHKNkfA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, Feb 2, 2012 at 7:26 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> Well, you can obviously check the catalogs for that, but you must be
> assuming that you don't have access to the catalogs or this would be a
> non-issue.
>
> You can also identify the kind of page by looking at the special area of the
> stored page. See:
> http://archives.postgresql.org/pgsql-hackers/2007-04/msg00392.php

How does that work with different forks?

I think its very ugly to mark all sorts of different pages as if they
were heap pages when they clearly aren't. I don't recall anything so
ugly being allowed anywhere else in the system. Why is it *needed*
here?

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Jim Nasby <jim(at)nasby(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring log_newpage
Date: 2012-02-02 09:47:43
Message-ID: 4F2A5BBF.7020900@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 02.02.2012 11:35, Simon Riggs wrote:
> On Thu, Feb 2, 2012 at 7:26 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>
>> Well, you can obviously check the catalogs for that, but you must be
>> assuming that you don't have access to the catalogs or this would be a
>> non-issue.
>>
>> You can also identify the kind of page by looking at the special area of the
>> stored page. See:
>> http://archives.postgresql.org/pgsql-hackers/2007-04/msg00392.php
>
> How does that work with different forks?

You have the fork number explicitly in the newpage record already.

> I think its very ugly to mark all sorts of different pages as if they
> were heap pages when they clearly aren't. I don't recall anything so
> ugly being allowed anywhere else in the system. Why is it *needed*
> here?

It's not needed. Beauty is in the eye of the beholder, but I don't find
it all that ugly, and the comment in log_newpage explains it well.

I don't see much value in adding a new field to the record. Any tools
that read the WAL format will need to be taught about that change. Not a
huge issue, but I also don't see much gain. On the whole I'd be inclined
to just leave it all alone, but whatever.

I don't think it's a good idea to rename XLOG_HEAP_NEWPAGE to
XLOG_NEWPAGE. The WAL record is still part of the heapam rmgr, even if
it's used by other access methods, too.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Jim Nasby <jim(at)nasby(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring log_newpage
Date: 2012-02-02 16:04:46
Message-ID: 24908.1328198686@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Wed, Feb 1, 2012 at 10:42 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
>> But we already had RelFileNode; wouldn't that be enough to tell what rmgr was responsible for the new page? Can 2 different rmgrs write to the same file node?

> No, but which one? No way to tell unless you have full list of
> relfilenodes and check each one. So btree changes look like heap
> changes etc..

What I'm not following is why we should care. There are no cases at
present where this matters, and no proposed patches (that I know of)
that would make it matter. If an individual rmgr needs control of
what happens during a given operation, it wouldn't use XLOG_NEWPAGE
to represent the operation. End of problem. You haven't provided
any convincing argument why this needs to be changed globally.

regards, tom lane