Re: [COMMITTERS] pgsql: Unite ReadBufferWithFork, ReadBufferWithStrategy, and

Log Message:
-----------
Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
functions into one ReadBufferExtended function, that takes the strategy
and mode as argument. There's three modes, RBM_NORMAL which is the default
used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
without throwing an error. The FSM needs the new mode to recover from
corrupt pages, which could happend if we crash after extending an FSM file,
and the new page is "torn".

Add fork number to some error messages in bufmgr.c, that still lacked it.

Modified Files:
--------------
pgsql/contrib/pageinspect:
rawpage.c (r1.8 -> r1.9)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/pageinspect/rawpage.c?r1=1.8&r2=1.9)
pgsql/src/backend/access/gin:
ginvacuum.c (r1.23 -> r1.24)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/gin/ginvacuum.c?r1=1.23&r2=1.24)
pgsql/src/backend/access/gist:
gistvacuum.c (r1.38 -> r1.39)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/gist/gistvacuum.c?r1=1.38&r2=1.39)
pgsql/src/backend/access/hash:
hashpage.c (r1.77 -> r1.78)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/hash/hashpage.c?r1=1.77&r2=1.78)
pgsql/src/backend/access/heap:
heapam.c (r1.266 -> r1.267)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/heap/heapam.c?r1=1.266&r2=1.267)
pgsql/src/backend/access/nbtree:
nbtree.c (r1.163 -> r1.164)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/nbtree/nbtree.c?r1=1.163&r2=1.164)
pgsql/src/backend/access/transam:
xlog.c (r1.320 -> r1.321)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.320&r2=1.321)
xlogutils.c (r1.59 -> r1.60)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlogutils.c?r1=1.59&r2=1.60)
pgsql/src/backend/commands:
analyze.c (r1.125 -> r1.126)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/analyze.c?r1=1.125&r2=1.126)
vacuum.c (r1.378 -> r1.379)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/vacuum.c?r1=1.378&r2=1.379)
vacuumlazy.c (r1.108 -> r1.109)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/vacuumlazy.c?r1=1.108&r2=1.109)
pgsql/src/backend/storage/buffer:
bufmgr.c (r1.239 -> r1.240)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/buffer/bufmgr.c?r1=1.239&r2=1.240)
pgsql/src/backend/storage/freespace:
freespace.c (r1.64 -> r1.65)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/freespace/freespace.c?r1=1.64&r2=1.65)
pgsql/src/include/access:
xlogutils.h (r1.26 -> r1.27)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/xlogutils.h?r1=1.26&r2=1.27)
pgsql/src/include/storage:
bufmgr.h (r1.115 -> r1.116)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/storage/bufmgr.h?r1=1.115&r2=1.116)

On Fri, 2008-10-31 at 15:05 +0000, Heikki Linnakangas wrote:
> Log Message:
> -----------
> Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
> functions into one ReadBufferExtended function, that takes the strategy
> and mode as argument. There's three modes, RBM_NORMAL which is the default
> used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
> a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
> without throwing an error. The FSM needs the new mode to recover from
> corrupt pages, which could happend if we crash after extending an FSM file,
> and the new page is "torn".

I thought you were adding the "read buffer only if in cache" option
also?

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Fri, 2008-10-31 at 15:05 +0000, Heikki Linnakangas wrote:
>> Log Message:
>> -----------
>> Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
>> functions into one ReadBufferExtended function, that takes the strategy
>> and mode as argument. There's three modes, RBM_NORMAL which is the default
>> used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
>> a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
>> without throwing an error. The FSM needs the new mode to recover from
>> corrupt pages, which could happend if we crash after extending an FSM file,
>> and the new page is "torn".
>
> I thought you were adding the "read buffer only if in cache" option
> also?

No, but if it's needed, it should now fit well into the infrastructure,
as a new ReadBuffer mode.

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

On Sat, 2008-11-01 at 15:18 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Fri, 2008-10-31 at 15:05 +0000, Heikki Linnakangas wrote:
> >> Log Message:
> >> -----------
> >> Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
> >> functions into one ReadBufferExtended function, that takes the strategy
> >> and mode as argument. There's three modes, RBM_NORMAL which is the default
> >> used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
> >> a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
> >> without throwing an error. The FSM needs the new mode to recover from
> >> corrupt pages, which could happend if we crash after extending an FSM file,
> >> and the new page is "torn".
> >
> > I thought you were adding the "read buffer only if in cache" option
> > also?
>
> No, but if it's needed, it should now fit well into the infrastructure,
> as a new ReadBuffer mode.

Not sure how this helps me; maybe it wasn't supposed to? I need to
implement XLogReadBufferExtended to take a cleanup lock. It seems wrong
to make that a ReadBufferMode, since the option refers to what happens
after we do ReadBuffer and especially because we need to do RBM_ZERO and
Cleanup mode at same time.

It would be much better to have ReadBufferExtended take a lockmode
argument also.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Sat, 2008-11-01 at 15:18 +0200, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>>> On Fri, 2008-10-31 at 15:05 +0000, Heikki Linnakangas wrote:
>>>> Log Message:
>>>> -----------
>>>> Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
>>>> functions into one ReadBufferExtended function, that takes the strategy
>>>> and mode as argument. There's three modes, RBM_NORMAL which is the default
>>>> used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
>>>> a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
>>>> without throwing an error. The FSM needs the new mode to recover from
>>>> corrupt pages, which could happend if we crash after extending an FSM file,
>>>> and the new page is "torn".
>>> I thought you were adding the "read buffer only if in cache" option
>>> also?
>> No, but if it's needed, it should now fit well into the infrastructure,
>> as a new ReadBuffer mode.
>
> Not sure how this helps me; maybe it wasn't supposed to?

Well, no, not directly. it was to plug the hole in the FSM
implementation with torn pages, and to make the ReadBuffer interface nicer.

> I need to
> implement XLogReadBufferExtended to take a cleanup lock. It seems wrong
> to make that a ReadBufferMode, since the option refers to what happens
> after we do ReadBuffer and especially because we need to do RBM_ZERO and
> Cleanup mode at same time.
>
> It would be much better to have ReadBufferExtended take a lockmode
> argument also.

No, I don't think we should mix ReadBuffer with the locking. On the
contrary, my suggestion is to modify XLogReadBufferExtended so that it
doesn't lock the page either, and leave the locking to the callers.

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

>> On Fri, 2008-10-31 at 15:05 +0000, Heikki Linnakangas wrote:
>>> Log Message:
>>> -----------
>>> Unite ReadBufferWithFork, ReadBufferWithStrategy, and ZeroOrReadBuffer
>>> functions into one ReadBufferExtended function, that takes the strategy
>>> and mode as argument. There's three modes, RBM_NORMAL which is the default
>>> used by plain ReadBuffer(), RBM_ZERO, which replaces ZeroOrReadBuffer, and
>>> a new mode RBM_ZERO_ON_ERROR, which allows callers to read corrupt pages
>>> without throwing an error. The FSM needs the new mode to recover from
>>> corrupt pages, which could happend if we crash after extending an FSM file,
>>> and the new page is "torn".

Hmm. I see that some messages are now like this:

(errmsg("unexpected data beyond EOF in block %u of relation %u/%u/%u/%u",
blockNum, smgr->smgr_rnode.spcNode, smgr->smgr_rnode.dbNode, smgr->smgr_rnode.relNode, forkNum),

but it seems that the file names contain symbolic fork names, not
numbers. Is it possible to build the error messages so that they report
the actual file name, or at least change the last /%u into a _%s with
the fork name?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Hmm. I see that some messages are now like this:
>
> (errmsg("unexpected data beyond EOF in block %u of relation %u/%u/%u/%u",
> blockNum, smgr->smgr_rnode.spcNode, smgr->smgr_rnode.dbNode, smgr->smgr_rnode.relNode, forkNum),
>
> but it seems that the file names contain symbolic fork names, not
> numbers. Is it possible to build the error messages so that they report
> the actual file name, or at least change the last /%u into a _%s with
> the fork name?

Agreed. There was some messages like that before, this patch just
changed some error messages I had missed before. I'll change them all to
match the file names ("%u/%u/%u_%s").

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

Heikki Linnakangas wrote:
> Alvaro Herrera wrote:
>> Hmm. I see that some messages are now like this:
>>
>> (errmsg("unexpected data beyond EOF in block %u of relation %u/%u/%u/%u",
>> blockNum, smgr->smgr_rnode.spcNode, smgr->smgr_rnode.dbNode,
>> smgr->smgr_rnode.relNode, forkNum),
>>
>> but it seems that the file names contain symbolic fork names, not
>> numbers. Is it possible to build the error messages so that they report
>> the actual file name, or at least change the last /%u into a _%s with
>> the fork name?
>
> Agreed. There was some messages like that before, this patch just
> changed some error messages I had missed before. I'll change them all to
> match the file names ("%u/%u/%u_%s").

I guess I should just change those error messages to use relpath().
relpath() seems perfectly safe to call from those places; there's e.g.
no catalog access.

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