Re: recovering from "found xmin ... from before relfrozenxid ..."

Ashutosh Sharma <ashu.coek88@gmail.com>

From: Ashutosh Sharma <ashu.coek88@gmail.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>, "Andrey M. Borodin" <x4mmm@yandex-team.ru>, MBeena Emerson <mbeena.emerson@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Andres Freund <andres@anarazel.de>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, Tom Lane <tgl@sss.pgh.pa.us>, Stephen Frost <sfrost@snowman.net>
Date: 2020-08-26T11:36:09Z
Lists: pgsql-hackers

Attachments

Thanks for the review. Please find my comments inline below:

On Tue, Aug 25, 2020 at 5:47 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> Let me share other comments on the latest version patch:
>
> Some words need to be tagged. For instance, I found the following words:
>
> VACUUM
> DISABLE_PAGE_SKIPPING
> HEAP_XMIN_FROZEN
> HEAP_XMAX_INVALID
>

Okay, done.

> ---
> +test=# select ctid from t1 where xmin = 507;
> + ctid
> +-------
> + (0,3)
> +(1 row)
> +
> +test=# select heap_force_freeze('t1'::regclass, ARRAY['(0, 3)']::tid[]);
> +-[ RECORD 1 ]-----+-
> +heap_force_freeze |
>
> I think it's better to use a consistent output format. The former uses
> the normal format whereas the latter uses the expanded format.
>

Yep, makes sense, done.

> ---
> + <note>
> + <para>
> +  While performing surgery on a damaged relation, we must not be doing anything
> +  else on that relation in parallel. This is to ensure that when we are
> +  operating on a damaged tuple there is no other transaction trying to modify
> +  that tuple.
> + </para>
> + </note>
>
> If we prefer to avoid concurrent operations on the target relation why
> don't we use AccessExclusiveLock?
>

Removed this note from the documentation and added a note saying: "The
user needs to ensure that they do not operate pg_force_freeze function
on a deleted tuple because it may revive the deleted tuple."

> ---
> +CREATE FUNCTION heap_force_kill(reloid regclass, tids tid[])
> +RETURNS VOID
> +AS 'MODULE_PATHNAME', 'heap_force_kill'
> +LANGUAGE C STRICT;
>
> +CREATE FUNCTION heap_force_freeze(reloid regclass, tids tid[])
> +RETURNS VOID
> +AS 'MODULE_PATHNAME', 'heap_force_freeze'
> +LANGUAGE C STRICT;
>
> I think these functions should be PARALLEL UNSAFE.
>

By default the functions are marked PARALLEL UNSAFE, so I think there
is nothing to do here.

Attached patch with above changes.

This patch also takes care of all the other review comments from - [1].

[1] - https://www.postgresql.org/message-id/CA%2Bfd4k6%2BJWq2MfQt5b7fSJ2wMvCes9TRfbDhVO_fQP9B8JJRAA%40mail.gmail.com


--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Commits

  1. Fix wrong data table horizon computation during backend startup.

  2. Centralize horizon determination for temp tables, fixing bug due to skew.

  3. pg_surgery: Try to stabilize regression tests.

  4. New contrib module, pg_surgery, with heap surgery functions.

  5. Set cutoff xmin more aggressively when vacuuming a temporary table.

  6. snapshot scalability: Don't compute global horizons while building snapshots.

  7. Introduce vacuum errcontext to display additional information.