Re: Small TRUNCATE glitch

Just noticed that TRUNCATE fails to clear the stats collector's counts
for the table. I am not sure if it should reset the event counts or
not (any thoughts?) but surely it is wrong to not zero the live/dead
tuple counts.

regards, tom lane

On Thu, Apr 03, 2008 at 11:58:11AM -0400, Tom Lane wrote:
> Just noticed that TRUNCATE fails to clear the stats collector's counts
> for the table. I am not sure if it should reset the event counts or
> not (any thoughts?) but surely it is wrong to not zero the live/dead
> tuple counts.

Wern't there complaints from people regularly truncating and
refilling tables getting bad plans because they lost the statistics?

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> On Thu, Apr 03, 2008 at 11:58:11AM -0400, Tom Lane wrote:
>> Just noticed that TRUNCATE fails to clear the stats collector's counts
>> for the table. I am not sure if it should reset the event counts or
>> not (any thoughts?) but surely it is wrong to not zero the live/dead
>> tuple counts.

> Wern't there complaints from people regularly truncating and
> refilling tables getting bad plans because they lost the statistics?

Not related --- the planner doesn't look at pgstats data.

regards, tom lane

Tom Lane wrote:
> Just noticed that TRUNCATE fails to clear the stats collector's counts
> for the table. I am not sure if it should reset the event counts or
> not (any thoughts?) but surely it is wrong to not zero the live/dead
> tuple counts.

Agreed, the live/dead counters should be reset. Regarding event counts,
my take is that we should have a separate statement count for truncate
(obviously not a tuple count), and the others should be left alone.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> Just noticed that TRUNCATE fails to clear the stats collector's counts
>> for the table. I am not sure if it should reset the event counts or
>> not (any thoughts?) but surely it is wrong to not zero the live/dead
>> tuple counts.

> Agreed, the live/dead counters should be reset. Regarding event counts,
> my take is that we should have a separate statement count for truncate
> (obviously not a tuple count), and the others should be left alone.

I thought some more about how to do it, and stumbled over how to cope
with TRUNCATE being rolled back. That nixed my first idea of just
having TRUNCATE send a zero-the-counters-now message.

regards, tom lane

Added to TODO:

o Clear table counters on TRUNCATE

http://archives.postgresql.org/pgsql-hackers/2008-04/msg00169.php

---------------------------------------------------------------------------

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Tom Lane wrote:
> >> Just noticed that TRUNCATE fails to clear the stats collector's counts
> >> for the table. I am not sure if it should reset the event counts or
> >> not (any thoughts?) but surely it is wrong to not zero the live/dead
> >> tuple counts.
>
> > Agreed, the live/dead counters should be reset. Regarding event counts,
> > my take is that we should have a separate statement count for truncate
> > (obviously not a tuple count), and the others should be left alone.
>
> I thought some more about how to do it, and stumbled over how to cope
> with TRUNCATE being rolled back. That nixed my first idea of just
> having TRUNCATE send a zero-the-counters-now message.
>
> regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
>
> Added to TODO:
>
> o Clear table counters on TRUNCATE
>
> http://archives.postgresql.org/pgsql-hackers/2008-04/msg00169.php

Hello,

Attached is a WIP patch for this TODO.

Alex Shulgin <ash(at)commandprompt(dot)com> writes:
>
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>>
>> Added to TODO:
>>
>> o Clear table counters on TRUNCATE
>>
>> http://archives.postgresql.org/pgsql-hackers/2008-04/msg00169.php
>
> Hello,
>
> Attached is a WIP patch for this TODO.

This part went as an attachment, which wasn't my intent:
========================================================

It does the trick by tracking if a TRUNCATE command was issued under a
(sub)transaction and uses this knowledge to reset the live/dead tuple
counters later if the transaction was committed. Testing in simple
cases shows that this clears the counters correctly, including use of
savepoints.

The 2PC part requires extending bool flag to fit the trunc flag, is this
approach sane? Given that 2PC transaction should survive server
restart, it's reasonable to expect it to also survive the upgrade, so I
see no clean way of adding another bool field to the
TwoPhasePgStatRecord struct (unless some would like to add checks on
record length, etc.).

I'm going to add some regression tests, but not sure what would be the
best location for this. The truncate.sql seems like natural choice, but
stats are not updating realtime, so I'd need to borrow some tricks from
stats.sql or better put the new tests in the stats.sql itself?

--
Alex

Alex Shulgin wrote:

> The 2PC part requires extending bool flag to fit the trunc flag, is this
> approach sane? Given that 2PC transaction should survive server
> restart, it's reasonable to expect it to also survive the upgrade, so I
> see no clean way of adding another bool field to the
> TwoPhasePgStatRecord struct (unless some would like to add checks on
> record length, etc.).

I don't think we need to have 2PC files survive a pg_upgrade. It seems
perfectly okay to remove them from the new cluster at some appropriate
time, *if* they are copied from the old cluster at all (I don't think
they should be.)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/10/2014 03:04 AM, Alvaro Herrera wrote:
> Alex Shulgin wrote:
>
>> The 2PC part requires extending bool flag to fit the trunc flag, is this
>> approach sane? Given that 2PC transaction should survive server
>> restart, it's reasonable to expect it to also survive the upgrade, so I
>> see no clean way of adding another bool field to the
>> TwoPhasePgStatRecord struct (unless some would like to add checks on
>> record length, etc.).
>
> I don't think we need to have 2PC files survive a pg_upgrade. It seems
> perfectly okay to remove them from the new cluster at some appropriate
> time, *if* they are copied from the old cluster at all (I don't think
> they should be.)

I think pg_upgrade should check if there are any prepared transactions
pending, and refuse to upgrade if there are. It could be made to work,
but it's really not worth the trouble. If there are any pending prepared
transactions in the system when you run pg_upgrade, it's more likely to
be a mistake or oversight in the first place, than on purpose.

- Heikki

On Wed, Dec 10, 2014 at 10:32:42AM +0200, Heikki Linnakangas wrote:
> >I don't think we need to have 2PC files survive a pg_upgrade. It seems
> >perfectly okay to remove them from the new cluster at some appropriate
> >time, *if* they are copied from the old cluster at all (I don't think
> >they should be.)
>
> I think pg_upgrade should check if there are any prepared
> transactions pending, and refuse to upgrade if there are. It could
> be made to work, but it's really not worth the trouble. If there are
> any pending prepared transactions in the system when you run
> pg_upgrade, it's more likely to be a mistake or oversight in the
> first place, than on purpose.

pg_upgrade already checks for prepared transactions and errors out if
they exist; see check_for_prepared_transactions().

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:

> On Wed, Dec 10, 2014 at 10:32:42AM +0200, Heikki Linnakangas wrote:
>> >I don't think we need to have 2PC files survive a pg_upgrade. It seems
>> >perfectly okay to remove them from the new cluster at some appropriate
>> >time, *if* they are copied from the old cluster at all (I don't think
>> >they should be.)
>>
>> I think pg_upgrade should check if there are any prepared
>> transactions pending, and refuse to upgrade if there are. It could
>> be made to work, but it's really not worth the trouble. If there are
>> any pending prepared transactions in the system when you run
>> pg_upgrade, it's more likely to be a mistake or oversight in the
>> first place, than on purpose.
>
> pg_upgrade already checks for prepared transactions and errors out if
> they exist; see check_for_prepared_transactions().

Alright, that's good to know. So I'm just adding a new bool field to
the 2PC pgstat record instead of the bit magic.

Attached is v0.2, now with a regression test included.

--
Alex