Re: [PATCHES] Writing WAL for relcache invalidation:pg_internal.init

Enclose a patch for new WAL records for relcache invalidation.

Not conclusively tested, but seems to work fine, so please regard this
as a prototype-needs-review.

We definitely need a regr test framework to support this type of patch.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> Enclose a patch for new WAL records for relcache invalidation.

I don't think this works. RelationCacheInitFileInvalidate is executed
post-commit, which means that there's a window between commit and where
you propose to write the WAL entry. A crash and restart in that
interval would leave the catalog changes committed, but not reflected
into pg_internal.init.

I think we're probably better off to just forcibly remove the init file
during post-recovery cleanup. The easiest place to do this might be
BuildFlatFiles, which has to scan pg_database anyway ...

regards, tom lane

On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote:

> I think we're probably better off to just forcibly remove the init file
> during post-recovery cleanup. The easiest place to do this might be
> BuildFlatFiles, which has to scan pg_database anyway ...

Presumably not rebuilding it, since we can let that occur naturally.

I'll submit a patch tomorrow.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > Enclose a patch for new WAL records for relcache invalidation.
>
> I don't think this works. RelationCacheInitFileInvalidate is executed
> post-commit, which means that there's a window between commit and where
> you propose to write the WAL entry. A crash and restart in that
> interval would leave the catalog changes committed, but not reflected
> into pg_internal.init.

Surely you are pointing out a bug, no?

If a backend did crash, the init file would be wrong and we'd get
exactly the same wrong relfilenode errors we got after that PITR.

The issue must surely be that the patch isn't wrong per se, just that
RelationCacheInitFileInvalidate is called too late and that requires an
additional fix. Are we certain that a crash between commit and
invalidation will cause a PANIC that takes down the server? Doesn't look
like its in a critical section to me.

> I think we're probably better off to just forcibly remove the init file
> during post-recovery cleanup. The easiest place to do this might be
> BuildFlatFiles, which has to scan pg_database anyway ...

I can do this - I don't have a problem there, but the above issue just
occurred to me so I wonder now if its the right thing to do.

PITR will be always-safe but normal operation might not be.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Tom, Simon et al; Please clarify.

PostgreSQL 8.1.5 on sparc-sun-solaris2.9, compiled by GCC gcc (GCC) 3.4.2

We're getting ready to init a new warm standby instance based on last
night's snapshot of running prod server. I see a few of these
pg_internal.init files in the cluster as it's being unpacked.

Same warm standby instance may sit for weeks gobbling up WALs from the
prod server then be finally brought live.

Question;

Is it safe to delete the .init files now (before starting recovery),
or perhaps unconditionally right after going live?

In other words, is there any sure fire preventative measure that I can
apply to guarantee against the fault that started this threadd?

Tom wrote:
> Meanwhile, if you're trying to recover from a PITR backup and it's not
> working, try removing any pg_internal.init files you can find.

Comment above seems to suggest not touching existing pg_internal.init
files unless a problem is seen.

Thanks

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:

> On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote:
>
> > "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > > Enclose a patch for new WAL records for relcache invalidation.
> >
> > I don't think this works. RelationCacheInitFileInvalidate is executed
> > post-commit, which means that there's a window between commit and where
> > you propose to write the WAL entry. A crash and restart in that
> > interval would leave the catalog changes committed, but not reflected
> > into pg_internal.init.
>
> Surely you are pointing out a bug, no?
>
> If a backend did crash, the init file would be wrong and we'd get
> exactly the same wrong relfilenode errors we got after that PITR.
>
> The issue must surely be that the patch isn't wrong per se, just that
> RelationCacheInitFileInvalidate is called too late and that requires an
> additional fix. Are we certain that a crash between commit and
> invalidation will cause a PANIC that takes down the server? Doesn't look
> like its in a critical section to me.
>
> > I think we're probably better off to just forcibly remove the init file
> > during post-recovery cleanup. The easiest place to do this might be
> > BuildFlatFiles, which has to scan pg_database anyway ...
>
> I can do this - I don't have a problem there, but the above issue just
> occurred to me so I wonder now if its the right thing to do.
>
> PITR will be always-safe but normal operation might not be.
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
>

--
-------------------------------------------------------------------------------
Jerry Sievers 305 854-3001 (home) WWW ECommerce Consultant
305 321-1144 (mobile http://www.JerrySievers.com/

On Thu, 2006-11-02 at 09:36 -0500, Jerry Sievers wrote:

> Is it safe to delete the .init files now (before starting recovery),
> or perhaps unconditionally right after going live?

You're safe to delete those files from the restored version of your base
backup prior to commencing startup with a recovery.conf.

When they are missing they will be recreated automatically.

When we agree on how to fix it, which should be soon, I would imagine
that the fix can be back patched to 8.0 and 8.1 and fixed prior to
release in 8.2.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote:
>> I don't think this works.

> Surely you are pointing out a bug, no?

> If a backend did crash, the init file would be wrong and we'd get
> exactly the same wrong relfilenode errors we got after that PITR.

Yeah, which is the same bug we've got now. They're both WAL-replay-
doesn't-fix-the-init-file cases.

> The issue must surely be that the patch isn't wrong per se, just that
> RelationCacheInitFileInvalidate is called too late and that requires an
> additional fix.

No. In the non-crash situation there is sufficient interlocking to
avoid a problem, and I feel no desire to redesign that mechanism.
Trying to do it before commit would create its own issues, anyway:
someone could install a new init file before you finish committing.

regards, tom lane

Jerry Sievers <jerry(at)jerrysievers(dot)com> writes:
> Is it safe to delete the .init files now (before starting recovery),

Should be OK as far as I can see.

regards, tom lane

On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote:

> I think we're probably better off to just forcibly remove the init file
> during post-recovery cleanup. The easiest place to do this might be
> BuildFlatFiles, which has to scan pg_database anyway ...

Patch enclosed.

Clean apply to HEAD, make check OK, plus restart check.
No specific PITR test, since same code path.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> On Wed, 2006-11-01 at 12:05 -0500, Tom Lane wrote:
>> I think we're probably better off to just forcibly remove the init file
>> during post-recovery cleanup. The easiest place to do this might be
>> BuildFlatFiles, which has to scan pg_database anyway ...

> Patch enclosed.

Applied to HEAD and 8.1. It doesn't work in 8.0 though, because
flatfiles.c doesn't exist. AFAIR there isn't any startup-time scan
of pg_database at all before 8.1. Not sure if it's worth coming up
with a whole new patch for that branch.

regards, tom lane