On 11 December 2012 03:01, Noah Misch <n...@leadboat.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
