Re: Re: [COMMITTERS] pgsql: Assorted cleanups in preparation for using a map file to support

Log Message:
-----------
Assorted cleanups in preparation for using a map file to support altering
the relfilenode of currently-not-relocatable system catalogs.

1. Get rid of inval.c's dependency on relfilenode, by not having it emit
smgr invalidations as a result of relcache flushes. Instead, smgr sinval
messages are sent directly from smgr.c when an actual relation delete or
truncate is done. This makes considerably more structural sense and allows
elimination of a large number of useless smgr inval messages that were
formerly sent even in cases where nothing was changing at the
physical-relation level. Note that this reintroduces the concept of
nontransactional inval messages, but that's okay --- because the messages
are sent by smgr.c, they will be sent in Hot Standby slaves, just from a
lower logical level than before.

2. Move setNewRelfilenode out of catalog/index.c, where it never logically
belonged, into relcache.c; which is a somewhat debatable choice as well but
better than before. (I considered catalog/storage.c, but that seemed too
low level.) Rename to RelationSetNewRelfilenode.

3. Cosmetic cleanups of some other relfilenode manipulations.

Modified Files:
--------------
pgsql/src/backend/catalog:
heap.c (r1.368 -> r1.369)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/heap.c?r1=1.368&r2=1.369)
index.c (r1.331 -> r1.332)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/index.c?r1=1.331&r2=1.332)
toasting.c (r1.28 -> r1.29)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/toasting.c?r1=1.28&r2=1.29)
pgsql/src/backend/commands:
tablecmds.c (r1.321 -> r1.322)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/tablecmds.c?r1=1.321&r2=1.322)
pgsql/src/backend/storage/smgr:
smgr.c (r1.118 -> r1.119)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/smgr/smgr.c?r1=1.118&r2=1.119)
pgsql/src/backend/utils/cache:
inval.c (r1.92 -> r1.93)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/cache/inval.c?r1=1.92&r2=1.93)
relcache.c (r1.300 -> r1.301)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/cache/relcache.c?r1=1.300&r2=1.301)
pgsql/src/include/catalog:
index.h (r1.80 -> r1.81)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/index.h?r1=1.80&r2=1.81)
pgsql/src/include/utils:
inval.h (r1.46 -> r1.47)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/inval.h?r1=1.46&r2=1.47)
relcache.h (r1.66 -> r1.67)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/relcache.h?r1=1.66&r2=1.67)

On Wed, 2010-02-03 at 01:14 +0000, Tom Lane wrote:

> 1. Get rid of inval.c's dependency on relfilenode, by not having it emit
> smgr invalidations as a result of relcache flushes. Instead, smgr sinval
> messages are sent directly from smgr.c when an actual relation delete or
> truncate is done. This makes considerably more structural sense and allows
> elimination of a large number of useless smgr inval messages that were
> formerly sent even in cases where nothing was changing at the
> physical-relation level. Note that this reintroduces the concept of
> nontransactional inval messages, but that's okay --- because the messages
> are sent by smgr.c, they will be sent in Hot Standby slaves, just from a
> lower logical level than before.

Presumably this means that SHAREDINVALSMGR_ID messages are no longer
part of the invalidation messages attached to a commit record?

If so, there is some minor code cleanup and comment changes in
ProcessCommittedInvalidationMessages(). Would you like me to do that, or
should we wait?

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Wed, 2010-02-03 at 01:14 +0000, Tom Lane wrote:
>> 1. Get rid of inval.c's dependency on relfilenode, by not having it emit
>> smgr invalidations as a result of relcache flushes. Instead, smgr sinval
>> messages are sent directly from smgr.c when an actual relation delete or
>> truncate is done. This makes considerably more structural sense and allows
>> elimination of a large number of useless smgr inval messages that were
>> formerly sent even in cases where nothing was changing at the
>> physical-relation level. Note that this reintroduces the concept of
>> nontransactional inval messages, but that's okay --- because the messages
>> are sent by smgr.c, they will be sent in Hot Standby slaves, just from a
>> lower logical level than before.

> Presumably this means that SHAREDINVALSMGR_ID messages are no longer
> part of the invalidation messages attached to a commit record?

Correct.

> If so, there is some minor code cleanup and comment changes in
> ProcessCommittedInvalidationMessages(). Would you like me to do that, or
> should we wait?

I saw that. I didn't touch it because it's not directly relevant to
what I'm doing right now, but I would like to go back and see whether
that routine can't be got rid of completely. It seems to me to be a
very klugy substitute for having enough information. I'm inclined to
think that we should emit an sinval message (or maybe better a separate
WAL entry) for initfile removal, instead of trying to reverse-engineer
whether one happened.

regards, tom lane

On Wed, 2010-02-03 at 10:48 -0500, Tom Lane wrote:

> > If so, there is some minor code cleanup and comment changes in
> > ProcessCommittedInvalidationMessages(). Would you like me to do that, or
> > should we wait?
>
> I saw that. I didn't touch it because it's not directly relevant to
> what I'm doing right now, but I would like to go back and see whether
> that routine can't be got rid of completely. It seems to me to be a
> very klugy substitute for having enough information. I'm inclined to
> think that we should emit an sinval message (or maybe better a separate
> WAL entry) for initfile removal, instead of trying to reverse-engineer
> whether one happened.

An additional sinval message type would work. There is a requirement for
us to run RelationCacheInitFileInvalidate() both before and after the
other messages. So we would need to append and prepend the new message
type onto the array of messages if transInvalInfo->RelcacheInitFileInval
is true. That way we would just do SendSharedInvalidMessages() in
xact_redo_commit and remove ProcessCommittedInvalidationMessages(),
adding other code to handle the inval message. Doesn't seem any easier
though.

Another WAL record would definitely be cumbersome.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Wed, 2010-02-03 at 10:48 -0500, Tom Lane wrote:
>>> If so, there is some minor code cleanup and comment changes in
>>> ProcessCommittedInvalidationMessages(). Would you like me to do that, or
>>> should we wait?
>>
>> I saw that. I didn't touch it because it's not directly relevant to
>> what I'm doing right now, but I would like to go back and see whether
>> that routine can't be got rid of completely. It seems to me to be a
>> very klugy substitute for having enough information. I'm inclined to
>> think that we should emit an sinval message (or maybe better a separate
>> WAL entry) for initfile removal, instead of trying to reverse-engineer
>> whether one happened.

> An additional sinval message type would work. There is a requirement for
> us to run RelationCacheInitFileInvalidate() both before and after the
> other messages. So we would need to append and prepend the new message
> type onto the array of messages if transInvalInfo->RelcacheInitFileInval
> is true. That way we would just do SendSharedInvalidMessages() in
> xact_redo_commit and remove ProcessCommittedInvalidationMessages(),
> adding other code to handle the inval message. Doesn't seem any easier
> though.

> Another WAL record would definitely be cumbersome.

BTW, we're definitely going to have to do *something* with that code,
because it's assuming that non-shared relcache init files always live in
DEFAULTTABLESPACE. That is not correct. I think that there is no
simple way for the startup process to identify which tablespace a given
database lives in (normally, one would have to consult pg_database to
find that out). So we are going to have to drive this off an sinval or
WAL record that does provide the tablespace as well as the DB OID.

regards, tom lane

On Thu, 2010-02-04 at 13:02 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Wed, 2010-02-03 at 10:48 -0500, Tom Lane wrote:
> >>> If so, there is some minor code cleanup and comment changes in
> >>> ProcessCommittedInvalidationMessages(). Would you like me to do that, or
> >>> should we wait?
> >>
> >> I saw that. I didn't touch it because it's not directly relevant to
> >> what I'm doing right now, but I would like to go back and see whether
> >> that routine can't be got rid of completely. It seems to me to be a
> >> very klugy substitute for having enough information. I'm inclined to
> >> think that we should emit an sinval message (or maybe better a separate
> >> WAL entry) for initfile removal, instead of trying to reverse-engineer
> >> whether one happened.
>
> > An additional sinval message type would work. There is a requirement for
> > us to run RelationCacheInitFileInvalidate() both before and after the
> > other messages. So we would need to append and prepend the new message
> > type onto the array of messages if transInvalInfo->RelcacheInitFileInval
> > is true. That way we would just do SendSharedInvalidMessages() in
> > xact_redo_commit and remove ProcessCommittedInvalidationMessages(),
> > adding other code to handle the inval message. Doesn't seem any easier
> > though.
>
> > Another WAL record would definitely be cumbersome.
>
> BTW, we're definitely going to have to do *something* with that code,
> because it's assuming that non-shared relcache init files always live in
> DEFAULTTABLESPACE. That is not correct.

Oh dear.

> I think that there is no
> simple way for the startup process to identify which tablespace a given
> database lives in (normally, one would have to consult pg_database to
> find that out). So we are going to have to drive this off an sinval or
> WAL record that does provide the tablespace as well as the DB OID.

Seems OK to just add the tablespace to the sinval.

--
Simon Riggs www.2ndQuadrant.com