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

On 11 December 2012 03:01, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Mon, Dec 10, 2012 at 08:04:55PM -0500, Robert Haas wrote:
>> I think the current behavior, where we treat FREEZE as a hint, is just
>> awful. Regardless of whether the behavior is automatic or manually
>> requested, the idea that you might get the optimization or not
>> depending on the timing of relcache flushes seems very much
>> undesirable. I mean, if the optimization is actually important for
>> performance, then you want to get it when you ask for it. If it
>> isn't, then why bother having it at all? Let's say that COPY FREEZE
>> normally doubles performance on a data load that therefore takes 8
>> hours - somebody who suddenly loses that benefit because of a relcache
>> flush that they can't prevent or control and ends up with a 16 hour
>> data load is going to pop a gasket.
>
> 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.

> 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.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services