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(at)leadboat(dot)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