Re: Commits 8de72b and 5457a1 (COPY FREEZE)

On Fri, Dec 21, 2012 at 06:47:56PM +0000, Simon Riggs wrote:
> On 11 December 2012 03:01, Noah Misch <noah@leadboat.com> wrote:
> > Until these threads, I did not know that a relcache invalidation could trip up
> > the WAL avoidance optimization, and now this.  I poked at the relevant
> > relcache.c code, and it already takes pains to preserve the needed facts.  The
> > header comment of RelationCacheInvalidate() indicates that entries bearing an
> > rd_newRelfilenodeSubid can safely survive the invalidation, but the code does
> > not implement that.  I think the comment is right, and this is just an
> > oversight in the code going back to its beginning (fba8113c).
> 
> I think the comment is right also and so the patch is good. I will
> apply, barring objections.
> 
> The information is only ever non-zero inside a single backend. There
> isn't any reason I can see why we wouldn't be able to remember this
> information in all cases, perhaps with a few push-ups.
> 
> > I doubt the comment at the declaration of rd_createSubid in rel.h, though I
> > can't presently say what correct comment should replace it.
> 
> rd_createSubid certainly does *not* get blown away by a message
> overflow as copy.c claims. I can't see any other way for a message
> overflow to cause it to be reset.
> 
> I can no longer see a reason for us to regard the rd_createSubid as
> merely a hint. So we should change copy.c also.

I thought of one case where we do currently forget rd_newRelfilenodeSubid:

BEGIN;
TRUNCATE t;
SAVEPOINT save;
TRUNCATE t;
ROLLBACK TO save;

I don't mind that one too much.

> > CLUSTER does
> > preserve the old value, at least for the main table relation.  CLUSTER
> > probably should *set* rd_newRelfilenodeSubid.
> 
> I can't see a reason not to do this in terms of correctness.
> 
> However, I can't see any reason why you'd want to CLUSTER a table and
> then load more data into it in the same transaction, so I'm tempted to
> just leave that as is to avoid introducing other bugs.
> 
> But I dare say people will think we should fix it there also.

I could see using that capability occasionally, but I wouldn't mix such a
change in with the goals of this thread.

Thanks,
nm