Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-18 Thread Robert Haas
On Tue, Jul 17, 2012 at 1:26 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Jul 16, 2012 at 9:58 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 BTW, I wonder whether the code that checks for relfilenode conflict
 when selecting a pg_class or relfilenode OID tries both file naming
 conventions?  If not, should we make it do so?

 I don't believe it does, nor do I see what we would gain by making it to do 
 so.

 What we would gain is ensuring that we aren't using the same relfilenode
 for both a regular table and a temp table.  Do you really want to assume
 that such a conflict is 100% safe?  It sounds pretty scary to me, and
 even if we were sure the backend would never get confused, what about
 client-side code that's looking at relfilenode?

Well, when I was working on that patch, I spent a lot of time trying
to ensure that it was in fact safe.  So I hope it is.  Also, note that
that could perfectly well happen anyway in any prior release if the
relations happened to live in different tablespaces.  Anyone assuming
that dboid,relfilenode is unique is kidding themselves, because it
is not guaranteed to be and has never been guaranteed to be.  Yes,
there could be client code out there that assumes
dboid,tsoid,relfilenode is unique and such code will need adjustment
for 9.1+.  But I bet there isn't very much.  The thing that broke a
lot of client code in that commit was the replacement of relistemp
with relpersistence; we already decided we didn't care about that (and
it's too late to change it now anyway) so I can't really get excited
about this.  I think that code assuming that anything other than a
RelFileNodeBackend is sufficient to uniquely identify a relation is
just buggy, and if there is any, we should fix it.  All remaining uses
of RelFileNode rather than RelFileNodeBackend should be cases where we
know that the backend ID has got to be InvalidBackendId.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-17 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Jul 16, 2012 at 9:58 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 BTW, I wonder whether the code that checks for relfilenode conflict
 when selecting a pg_class or relfilenode OID tries both file naming
 conventions?  If not, should we make it do so?

 I don't believe it does, nor do I see what we would gain by making it to do 
 so.

What we would gain is ensuring that we aren't using the same relfilenode
for both a regular table and a temp table.  Do you really want to assume
that such a conflict is 100% safe?  It sounds pretty scary to me, and
even if we were sure the backend would never get confused, what about
client-side code that's looking at relfilenode?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-17 Thread Tom Lane
I wrote:
 I had thought that we might get a performance boost here by saving fsync
 queue traffic, but I see that md.c was already not calling
 register_dirty_segment for temp rels, so there's no joy there.

Actually, wait a second.  We were smart enough to not send fsync
requests in the first place for temp rels.  But we were not smart enough
to not call ForgetRelationFsyncRequests when deleting a temp rel,
which made for an entirely useless scan through the pending-fsyncs
table.  So there could be win there, on top of not forwarding the actual
unlink operation.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Amit Kapila
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
Sent: Monday, July 16, 2012 3:06 AM

 It might have accidentally failed to fail if tested on a compiler that
 gives a full 32 bits to enum ForkNumber, but there absolutely would be
 padding there if ForkNumber is allocated as short or char.

 As best I can tell, a failure from uninitialized padding would not cause
 visible misbehavior but only make it not notice that two requests are
 identical, so that the queue compaction would not accomplish as much as
 it should.  Nonetheless, this seems pretty broken.

 We could fairly cheaply dodge the problem with padding after ForkNumber
 if we were to zero out the whole request array at shmem initialization,
 so that any such pad bytes are guaranteed zero.  However, padding in
 RelFileNodeBackend would be more annoying to deal with, and at least
 in the current instantiation of those structs it's probably impossible
 anyway.  Should we document those structs as required to not contain
 any padding, or do what's needful in checkpointer.c to not depend on
 there not being padding?

If we just document those structs, then how to handle the case where
ForkNumber
is allocated as short or char?



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Robert Haas
On Sun, Jul 15, 2012 at 5:36 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 We could fairly cheaply dodge the problem with padding after ForkNumber
 if we were to zero out the whole request array at shmem initialization,
 so that any such pad bytes are guaranteed zero.  However, padding in
 RelFileNodeBackend would be more annoying to deal with, and at least
 in the current instantiation of those structs it's probably impossible
 anyway.  Should we document those structs as required to not contain
 any padding, or do what's needful in checkpointer.c to not depend on
 there not being padding?

I would expect that every method we could devise for allocating a
shared memory segment would produce all-zero bytes.  There was a time
long ago when the OS would simply hand over previously-freed pages
with their existing contents, but I believe that was recognized as a
security problems more than 20 years ago - maybe 30 - and I can't
believe there is any OS we care about supporting that fails to zero
pages before handing them out.  Of course you can't count on malloc()
to return zero'd memory, but that's because the process might get a
page (all zeros) from the OS, allocate it, free it, and then
reallocate it for an unrelated purpose.  But we have no method that I
know of for freeing shared memory, and even if we did, the memory used
by the fsync queue is allocated during startup and therefore
presumably prior to any hypothetical ShmemFree operations that might
occur subsequently.

So I'm having a hard time understanding under what imaginable set of
circumstances this might break.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Sun, Jul 15, 2012 at 5:36 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 We could fairly cheaply dodge the problem with padding after ForkNumber
 if we were to zero out the whole request array at shmem initialization,
 so that any such pad bytes are guaranteed zero.  However, padding in
 RelFileNodeBackend would be more annoying to deal with, and at least
 in the current instantiation of those structs it's probably impossible
 anyway.  Should we document those structs as required to not contain
 any padding, or do what's needful in checkpointer.c to not depend on
 there not being padding?

 I would expect that every method we could devise for allocating a
 shared memory segment would produce all-zero bytes.

Well, it'd likely produce all-something bytes, but I don't believe
shmget is documented to produce zeroes.  In any case we are not in
the habit of relying on that and I don't see why we'd do so here rather
than explicitly zeroing the relatively small amount of memory involved.

 So I'm having a hard time understanding under what imaginable set of
 circumstances this might break.

Padding inside RelFileNodeBackend would break it, because
ForwardFsyncRequest copies the rnode as a struct.  So that's why I'm
asking whether we want to establish an explicit requirement that that
struct not contain any padding.

It would not be that hard to avoid the problem: we could make
CompactCheckpointerRequestQueue pre-zero a tag variable and then copy
the live fields into it.  Unless there is some other place in the system
that depends on RelFileNodeBackend being non-padded, and will break in a
more visible fashion with padding, I'm now inclined to do it like that.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Heikki Linnakangas

On 16.07.2012 18:24, Robert Haas wrote:

On Sun, Jul 15, 2012 at 5:36 PM, Tom Lanet...@sss.pgh.pa.us  wrote:

We could fairly cheaply dodge the problem with padding after ForkNumber
if we were to zero out the whole request array at shmem initialization,
so that any such pad bytes are guaranteed zero.  However, padding in
RelFileNodeBackend would be more annoying to deal with, and at least
in the current instantiation of those structs it's probably impossible
anyway.  Should we document those structs as required to not contain
any padding, or do what's needful in checkpointer.c to not depend on
there not being padding?


I would expect that every method we could devise for allocating a
shared memory segment would produce all-zero bytes.  There was a time
long ago when the OS would simply hand over previously-freed pages
with their existing contents, but I believe that was recognized as a
security problems more than 20 years ago - maybe 30 - and I can't
believe there is any OS we care about supporting that fails to zero
pages before handing them out.


I wouldn't rely on that, though. I wouldn't be surprised if there was 
some debugging flag or similar that initialized all pages to random 
values or 0xdeadbeef or something, before handing them out to the 
application. We could easily zero all shared memory on allocation 
ourselves, though.


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Tom Lane
I wrote:
 Robert Haas robertmh...@gmail.com writes:
 So I'm having a hard time understanding under what imaginable set of
 circumstances this might break.

 Padding inside RelFileNodeBackend would break it, because
 ForwardFsyncRequest copies the rnode as a struct.  So that's why I'm
 asking whether we want to establish an explicit requirement that that
 struct not contain any padding.

BTW, I'd be a lot happier about assuming that bare RelFileNode contains
no padding, because that's at least got all the fields the same type.
So that brings us back to the question of why this code is supporting
fsync requests for local relations in the first place.  Couldn't we have
it ignore those, and then only ship RelFileNode to the checkpointer?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 11:57 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I wrote:
 Robert Haas robertmh...@gmail.com writes:
 So I'm having a hard time understanding under what imaginable set of
 circumstances this might break.

 Padding inside RelFileNodeBackend would break it, because
 ForwardFsyncRequest copies the rnode as a struct.  So that's why I'm
 asking whether we want to establish an explicit requirement that that
 struct not contain any padding.

 BTW, I'd be a lot happier about assuming that bare RelFileNode contains
 no padding, because that's at least got all the fields the same type.
 So that brings us back to the question of why this code is supporting
 fsync requests for local relations in the first place.  Couldn't we have
 it ignore those, and then only ship RelFileNode to the checkpointer?

That's an awfully good point.  I think that was just sloppy coding on
my part (cf commit debcec7dc31a992703911a9953e299c8d730c778).  +1 for
changing it as you suggest.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Jul 16, 2012 at 11:57 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 BTW, I'd be a lot happier about assuming that bare RelFileNode contains
 no padding, because that's at least got all the fields the same type.
 So that brings us back to the question of why this code is supporting
 fsync requests for local relations in the first place.  Couldn't we have
 it ignore those, and then only ship RelFileNode to the checkpointer?

 That's an awfully good point.  I think that was just sloppy coding on
 my part (cf commit debcec7dc31a992703911a9953e299c8d730c778).  +1 for
 changing it as you suggest.

OK, so I think the current proposal is:

1. Document that RelFileNode must not contain padding.

2. Change the fsync forwarding code to ignore backend-local relations,
and include only RelFileNode not RelFileNodeBackend in requests.

3. Zero the checkpointer requests[] array at shmem init time, so as
to ensure consistency of any pad bytes elsewhere in the request structs.

I will see about making this happen.  Since the fsync queue compaction
code got back-patched awhile ago, we need to back-patch the relevant
parts of this too.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 11:44 AM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 I wouldn't rely on that, though. I wouldn't be surprised if there was some
 debugging flag or similar that initialized all pages to random values or
 0xdeadbeef or something, before handing them out to the application. We
 could easily zero all shared memory on allocation ourselves, though.

Well, the documentation for mmap (which we're currently using) on Linux says:

   MAP_ANONYMOUS
  The mapping is not backed by any file; its contents are initial‐
  ized to zero.  The fd and offset arguments are ignored; however,
  some implementations require fd to be -1  if  MAP_ANONYMOUS  (or
  MAP_ANON)  is specified, and portable applications should ensure
  this.  The use of MAP_ANONYMOUS in conjunction  with  MAP_SHARED
  is only supported on Linux since kernel 2.4.

shmget says:

   When  a new shared memory segment is created, its contents are initial‐
   ized to zero values, and its associated data structure,  shmid_ds  (see
   shmctl(2)), is initialized as follows:

And shm_open says:

  A new shared memory object initially has zero length  —  the
  size of the object can be set using ftruncate(2).  The newly
  allocated bytes of a shared memory object are  automatically
  initialized to 0.

The documentation on MacOS X isn't quite as explicit, but I'd still be
astonished if we found any other behavior.  TBH, I'd be kind of
surprised if this is the only place in our code base that relies on
the initial contents of shared memory being all-zeros.  If we really
care about that we probably ought to make --enable-cassert write
0xdeadbeef all over the whole shared-memory segment on startup, or
something like that, because otherwise it's only a matter of time
before someone will break it.  Personally I'd like to see some
evidence that the problem is more than strictly hypothetical before we
spend time on it, though.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 The documentation on MacOS X isn't quite as explicit, but I'd still be
 astonished if we found any other behavior.  TBH, I'd be kind of
 surprised if this is the only place in our code base that relies on
 the initial contents of shared memory being all-zeros.

Maybe so, but if we find any others, I'll be wanting to change them too.
It's bad practice and worse documentation for modules to be silently
assuming that anything has a value they didn't explicitly give it.

A related practice that probably costs us a lot more, in both code space
and time, is that most (all?) places that create Node objects explicitly
initialize every field of the Node struct, even though makeNode() has
a palloc0 underneath it and so setting fields to zero is redundant.
I believe that this is a good practice anyway, for documentation and
code greppability reasons.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 12:27 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 The documentation on MacOS X isn't quite as explicit, but I'd still be
 astonished if we found any other behavior.  TBH, I'd be kind of
 surprised if this is the only place in our code base that relies on
 the initial contents of shared memory being all-zeros.

 Maybe so, but if we find any others, I'll be wanting to change them too.
 It's bad practice and worse documentation for modules to be silently
 assuming that anything has a value they didn't explicitly give it.

 A related practice that probably costs us a lot more, in both code space
 and time, is that most (all?) places that create Node objects explicitly
 initialize every field of the Node struct, even though makeNode() has
 a palloc0 underneath it and so setting fields to zero is redundant.
 I believe that this is a good practice anyway, for documentation and
 code greppability reasons.

I don't really agree; I find it nice and clean to assume that
functions that promise to return zero'd memory really do.  In my book,
the main reason for keeping things as they are is that it's probably
not costing enough to matter very much.  Which is true here, too, so
I'm not going to make a huge stink, but I still think it's a waste of
effort.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Tom Lane
I wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Jul 16, 2012 at 11:57 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 So that brings us back to the question of why this code is supporting
 fsync requests for local relations in the first place.  Couldn't we have
 it ignore those, and then only ship RelFileNode to the checkpointer?

 That's an awfully good point.  I think that was just sloppy coding on
 my part (cf commit debcec7dc31a992703911a9953e299c8d730c778).  +1 for
 changing it as you suggest.

 2. Change the fsync forwarding code to ignore backend-local relations,
 and include only RelFileNode not RelFileNodeBackend in requests.

So I started to do this, and immediately hit a roadblock: although it's
clear that we can discard any attempt to fsync a backend-local relation,
it's not so clear that we don't need to queue UNLINK_RELATION_REQUEST
operations for local relations.

I think that this is in fact okay.  The reason for delaying unlink until
after the next checkpoint is that if we did not, and the relfilenode got
re-used for an unrelated relation, and then we crashed and had to replay
WAL, we would replay any WAL referencing the old relation into the
unrelated relation's storage, with potential bad consequences if you're
unlucky.  However, no WAL should ever be generated for a backend-local
relation, so there is nothing to guard against and hence no harm in
immediately unlinking backend-local rels when they are deleted.  So we
can adjust mdunlink to include SmgrIsTemp() among the reasons to unlink
immediately rather than doing the truncate-and-register_unlink dance.

If anybody sees a hole in this reasoning, speak now ...

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 2:47 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Jul 16, 2012 at 11:57 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 So that brings us back to the question of why this code is supporting
 fsync requests for local relations in the first place.  Couldn't we have
 it ignore those, and then only ship RelFileNode to the checkpointer?

 That's an awfully good point.  I think that was just sloppy coding on
 my part (cf commit debcec7dc31a992703911a9953e299c8d730c778).  +1 for
 changing it as you suggest.

 2. Change the fsync forwarding code to ignore backend-local relations,
 and include only RelFileNode not RelFileNodeBackend in requests.

 So I started to do this, and immediately hit a roadblock: although it's
 clear that we can discard any attempt to fsync a backend-local relation,
 it's not so clear that we don't need to queue UNLINK_RELATION_REQUEST
 operations for local relations.

 I think that this is in fact okay.  The reason for delaying unlink until
 after the next checkpoint is that if we did not, and the relfilenode got
 re-used for an unrelated relation, and then we crashed and had to replay
 WAL, we would replay any WAL referencing the old relation into the
 unrelated relation's storage, with potential bad consequences if you're
 unlucky.  However, no WAL should ever be generated for a backend-local
 relation, so there is nothing to guard against and hence no harm in
 immediately unlinking backend-local rels when they are deleted.  So we
 can adjust mdunlink to include SmgrIsTemp() among the reasons to unlink
 immediately rather than doing the truncate-and-register_unlink dance.

 If anybody sees a hole in this reasoning, speak now ...

Hmm, yeah, I have a feeling this might be why I didn't do this when I
created RelFileNodeBackend.  But I think your reasoning is correct.
Sticking the above text in a comment might not be out of order,
however.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Jul 16, 2012 at 2:47 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 So I started to do this, and immediately hit a roadblock: although it's
 clear that we can discard any attempt to fsync a backend-local relation,
 it's not so clear that we don't need to queue UNLINK_RELATION_REQUEST
 operations for local relations.
 
 I think that this is in fact okay.  The reason for delaying unlink until
 after the next checkpoint is that if we did not, and the relfilenode got
 re-used for an unrelated relation, and then we crashed and had to replay
 WAL, we would replay any WAL referencing the old relation into the
 unrelated relation's storage, with potential bad consequences if you're
 unlucky.  However, no WAL should ever be generated for a backend-local
 relation, so there is nothing to guard against and hence no harm in
 immediately unlinking backend-local rels when they are deleted.  So we
 can adjust mdunlink to include SmgrIsTemp() among the reasons to unlink
 immediately rather than doing the truncate-and-register_unlink dance.
 
 If anybody sees a hole in this reasoning, speak now ...

 Hmm, yeah, I have a feeling this might be why I didn't do this when I
 created RelFileNodeBackend.  But I think your reasoning is correct.
 Sticking the above text in a comment might not be out of order,
 however.

Most of this info was already in the comment for mdunlink, so I just
added a bit there.

The attached patch covers everything discussed in this thread, except
for the buggy handling of stats, which I think should be fixed in a
separate patch since it's only relevant to 9.2+.

I had thought that we might get a performance boost here by saving fsync
queue traffic, but I see that md.c was already not calling
register_dirty_segment for temp rels, so there's no joy there.  We might
win a bit by not queuing deletion of temp rels, though.  There's also
some distributed savings by eliminating one field from the request queue
and hash table, but probably not enough to notice.  I think the main
reason to do this is just to tighten up the question of whether pad
bytes matter.

Haven't started on back-patching yet.  Some but not all of this will
need to go back as far as we back-patched the queue compaction logic.

regards, tom lane

diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 417e3bb0d1b0c90632aafa5fbec351ab21f7e5a0..92fd4276cd1b3be81d1ac741f9f6ea09d241ea52 100644
*** a/src/backend/postmaster/checkpointer.c
--- b/src/backend/postmaster/checkpointer.c
***
*** 105,111 
   */
  typedef struct
  {
! 	RelFileNodeBackend rnode;
  	ForkNumber	forknum;
  	BlockNumber segno;			/* see md.c for special values */
  	/* might add a real request-type field later; not needed yet */
--- 105,111 
   */
  typedef struct
  {
! 	RelFileNode	rnode;
  	ForkNumber	forknum;
  	BlockNumber segno;			/* see md.c for special values */
  	/* might add a real request-type field later; not needed yet */
*** CheckpointerShmemSize(void)
*** 924,940 
  void
  CheckpointerShmemInit(void)
  {
  	bool		found;
  
  	CheckpointerShmem = (CheckpointerShmemStruct *)
  		ShmemInitStruct(Checkpointer Data,
! 		CheckpointerShmemSize(),
  		found);
  
  	if (!found)
  	{
! 		/* First time through, so initialize */
! 		MemSet(CheckpointerShmem, 0, sizeof(CheckpointerShmemStruct));
  		SpinLockInit(CheckpointerShmem-ckpt_lck);
  		CheckpointerShmem-max_requests = NBuffers;
  	}
--- 924,945 
  void
  CheckpointerShmemInit(void)
  {
+ 	Size		size = CheckpointerShmemSize();
  	bool		found;
  
  	CheckpointerShmem = (CheckpointerShmemStruct *)
  		ShmemInitStruct(Checkpointer Data,
! 		size,
  		found);
  
  	if (!found)
  	{
! 		/*
! 		 * First time through, so initialize.  Note that we zero the whole
! 		 * requests array; this is so that CompactCheckpointerRequestQueue
! 		 * can assume that any pad bytes in the request structs are zeroes.
! 		 */
! 		MemSet(CheckpointerShmem, 0, size);
  		SpinLockInit(CheckpointerShmem-ckpt_lck);
  		CheckpointerShmem-max_requests = NBuffers;
  	}
*** RequestCheckpoint(int flags)
*** 1091,1101 
   *		Forward a file-fsync request from a backend to the checkpointer
   *
   * Whenever a backend is compelled to write directly to a relation
!  * (which should be seldom, if the checkpointer is getting its job done),
   * the backend calls this routine to pass over knowledge that the relation
   * is dirty and must be fsync'd before next checkpoint.  We also use this
   * opportunity to count such writes for statistical purposes.
   *
   * segno specifies which segment (not block!) of the relation needs to be
   * fsync'd.  (Since the valid range is much less than BlockNumber, we can
   * use high values for special flags; that's all internal to md.c, which
--- 1096,1110 
   *		Forward a file-fsync request from a backend to the checkpointer
   *
   * 

Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 7:17 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 The attached patch covers everything discussed in this thread, except
 for the buggy handling of stats, which I think should be fixed in a
 separate patch since it's only relevant to 9.2+.

With respect to this chunk:

+  * We do not need to go through this dance for temp relations, though, because
+  * we never make WAL entries for temp rels, and so a temp rel poses no threat
+  * to the health of a regular rel that has taken over its relfilenode number.

...I would say that a clearer way to put this is that temporary
relations use a different file naming convention than permanent
relations and therefore there can never be any confusion between the
two.

Other than that, looks fine to me.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 With respect to this chunk:

 +  * We do not need to go through this dance for temp relations, though, 
 because
 +  * we never make WAL entries for temp rels, and so a temp rel poses no 
 threat
 +  * to the health of a regular rel that has taken over its relfilenode 
 number.

 ...I would say that a clearer way to put this is that temporary
 relations use a different file naming convention than permanent
 relations and therefore there can never be any confusion between the
 two.

Yeah, that's an entirely independent reason why there's probably no
issue in recent releases.  The rationale as stated is back-patchable
to earlier releases, though.

BTW, I wonder whether the code that checks for relfilenode conflict
when selecting a pg_class or relfilenode OID tries both file naming
conventions?  If not, should we make it do so?

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CompactCheckpointerRequestQueue versus pad bytes

2012-07-16 Thread Robert Haas
On Mon, Jul 16, 2012 at 9:58 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 BTW, I wonder whether the code that checks for relfilenode conflict
 when selecting a pg_class or relfilenode OID tries both file naming
 conventions?  If not, should we make it do so?

I don't believe it does, nor do I see what we would gain by making it to do so.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers