Re: -DCLOBBER_CACHE_ALWAYS shows COPY FREEZE regression problem

On a new buildfarm member friarbird
<http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2012-12-06%2020%3A55%3A31>,
configured with _DCLOBBER_CACHE_ALWAYS:

BEGIN;
TRUNCATE vistest;
COPY vistest FROM stdin CSV FREEZE;
+ NOTICE: FREEZE option specified but pre-conditions not met

and similar.

cheers

andrew

On 6 December 2012 23:28, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> On a new buildfarm member friarbird
> <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2012-12-06%2020%3A55%3A31>,
> configured with _DCLOBBER_CACHE_ALWAYS:
>
> BEGIN;
> TRUNCATE vistest;
> COPY vistest FROM stdin CSV FREEZE;
> + NOTICE: FREEZE option specified but pre-conditions not met

I don't understand why that build option would produce different
output for simple logic.

I'll look again in the morning.

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

On 2012-12-06 23:59:06 +0000, Simon Riggs wrote:
> On 6 December 2012 23:28, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> > On a new buildfarm member friarbird
> > <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2012-12-06%2020%3A55%3A31>,
> > configured with _DCLOBBER_CACHE_ALWAYS:
> >
> > BEGIN;
> > TRUNCATE vistest;
> > COPY vistest FROM stdin CSV FREEZE;
> > + NOTICE: FREEZE option specified but pre-conditions not met
>
> I don't understand why that build option would produce different
> output for simple logic.
>
> I'll look again in the morning.

It clears the relcache entry as soon as AcceptInvalidationMessages() is
done seems to loose rd_createSubid and rd_newRelfilenodeSubid.

Apparently the magic to preserve those values across cache resets isn't
strong enough for this. Seems bad, because that seems to mean a sinval
overflow leads to this and related optimizations being lost?

Greetings,

Andres Freund

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

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On a new buildfarm member friarbird
> <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2012-12-06%2020%3A55%3A31>,
> configured with _DCLOBBER_CACHE_ALWAYS:

> BEGIN;
> TRUNCATE vistest;
> COPY vistest FROM stdin CSV FREEZE;
> + NOTICE: FREEZE option specified but pre-conditions not met

The notice is coming out because the relcache is dropping the value of
rd_newRelfilenodeSubid as a result of cache flushes. The COPY FREEZE
code is aware of this risk, commenting

* As mentioned in comments in utils/rel.h, the in-same-transaction test
* is not completely reliable, since in rare cases rd_createSubid or
* rd_newRelfilenodeSubid can be cleared before the end of the transaction.
* However this is OK since at worst we will fail to make the optimization.

but I'd say this seriously throws into question whether it should be
emitting a notice. That seems to tie into the discussion a little bit
ago about whether the FREEZE option is a command or a hint. Throwing a
notice seems like a pretty schizophrenic choice: it's not an error but
you're in the user's face about it anyway. In any case, if the option
is unreliable (and with this implementation it certainly is), we can
*not* treat its failure as an error, so it's not a command.

TBH I think that COPY FREEZE should not have been committed yet;
it doesn't seem to be fully baked.

regards, tom lane

On 7 December 2012 00:06, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> Apparently the magic to preserve those values across cache resets isn't
> strong enough for this. Seems bad, because that seems to mean a sinval
> overflow leads to this and related optimizations being lost?

Which seems to back up the case for it being a hint, that the system
might not be able to honor.

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

On 7 December 2012 00:13, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On a new buildfarm member friarbird
>> <http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2012-12-06%2020%3A55%3A31>,
>> configured with _DCLOBBER_CACHE_ALWAYS:
>
>> BEGIN;
>> TRUNCATE vistest;
>> COPY vistest FROM stdin CSV FREEZE;
>> + NOTICE: FREEZE option specified but pre-conditions not met
>
> The notice is coming out because the relcache is dropping the value of
> rd_newRelfilenodeSubid as a result of cache flushes. The COPY FREEZE
> code is aware of this risk, commenting
>
> * As mentioned in comments in utils/rel.h, the in-same-transaction test
> * is not completely reliable, since in rare cases rd_createSubid or
> * rd_newRelfilenodeSubid can be cleared before the end of the transaction.
> * However this is OK since at worst we will fail to make the optimization.
>
> but I'd say this seriously throws into question whether it should be
> emitting a notice. That seems to tie into the discussion a little bit
> ago about whether the FREEZE option is a command or a hint. Throwing a
> notice seems like a pretty schizophrenic choice: it's not an error but
> you're in the user's face about it anyway. In any case, if the option
> is unreliable (and with this implementation it certainly is), we can
> *not* treat its failure as an error, so it's not a command.

Hmm, yes, its pretty schizophrenic, but that comes from my attempt to
offer something useful to the user in line with Robert's wish for the
hint to not be silently ignored. If I had overridden that and made it
truly silent as I had proposed, the clobber error would not have
happened.

I guess my mind must have been aware of the above, but it regrettably
wasn't consciously there. The above text was there from before, not
from the recent patch.

I'll remove the message now so tests pass, since that was my original
intention, but others may wish to suggest other ways forward.

> TBH I think that COPY FREEZE should not have been committed yet;
> it doesn't seem to be fully baked.

I think so too, in hindsight, but at the time it seemed a simple patch
that followed review and recent on-list discussion. My mistake was
tinkering with the patch before commit, which I then revoked.

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