Hi Michael,
Thanks for taking a look at the patch.
> twophase.c: In function ‘PrepareRedoAdd’:
> twophase.c:2539:20: warning: variable ‘gxact’ set but not used
> [-Wunused-but-set-variable]
> GlobalTransaction gxact;
> There is a warning at compilation.
>
>
Will fix.
> The comment at the top of PrescanPreparedTransactions() needs to be
> updated. Not only does this routine look for the contents of
> pg_twophase, but it does also look at the shared memory contents, at
> least those ones marked with inredo and not on_disk.
>
>
Oh yes. Will change comments at top of
StandbyRecoverPreparedTransactions(), RecoverPreparedTransactions() as well.
> + ereport(WARNING,
> + (errmsg("removing future two-phase state data from
> memory \"%u\"",
> + xid)));
> + PrepareRedoRemove(xid);
> + continue
> Those are not normal (partially because unlink is atomic, but not
> durable)... But they match the correct coding pattern regarding
> incorrect 2PC entries... I'd really like to see those switched to a
> FATAL with unlink() made durable for those calls.
>
>
Hmm, not sure what exactly we need to do here. If you look at the prior
checks, there we already skip on-disk entries. So, typically, the entries
that we encounter here will be in shmem only.
> + /* Deconstruct header */
> + hdr = (TwoPhaseFileHeader *) buf;
> + Assert(TransactionIdEquals(hdr->xid, xid));
> +
> + if (TransactionIdPrecedes(xid, result))
> + result = xid;
> This portion is repeated three times and could be easily refactored.
> You could just have a routine that returns the oldes transaction ID
> used, and ignore the result for StandbyRecoverPreparedTransactions()
> by casting a (void) to let any kind of static analyzer understand that
> we don't care about the result for example. Handling for subxids is
> necessary as well depending on the code path. Spliting things into a
> could of sub-routines may be more readable as well. There are really a
> couple of small parts that can be gathered and strengthened.
>
>
I will see if we can reduce this to a couple of function calls.
> + /*
> + * Recreate its GXACT and dummy PGPROC
> + */
> + gxactnew = MarkAsPreparing(xid, gid,
> + hdr->prepared_at,
> + hdr->owner, hdr->database,
> + gxact->prepare_start_lsn,
> + gxact->prepare_end_lsn);
> MarkAsPreparing() does not need to be extended with two new arguments.
> RecoverPreparedTransactions() is used only at the end of recovery,
> where it is not necessary to look at the 2PC state files from the
> records. In this code path inredo is also set to false :)
>
>
That's not true. We will have entries with inredo set at the end of
recovery as well. Infact the MarkAsPreparing() call
from RecoverPreparedTransactions() is the one which will remove these
inredo entries and convert them into regular entries. We have optimized the
recovery code path as well.
> + {
> + /*
> + * Entry could be on disk. Call with giveWarning=false
> + * since it can be expected during replay.
> + */
> + RemoveTwoPhaseFile(xid, false);
> + }
> This would be removed at the end of recovery anyway as a stale entry,
> so that's not necessary.
>
>
Ok, will remove this.
> + /* Delete TwoPhaseState gxact entry and/or 2PC file. */
> + PrepareRedoRemove(parsed.twophase_xid);
> Both things should not be present, no? If the file is pushed to disk
> it means that the checkpoint horizon has already moved.
>
>
PREPARE in redo, followed by a checkpoint, followed by a COMMIT/ROLLBACK.
We can have both the bits set in this case.
> - ereport(ERROR,
> + /* It's ok to find an entry in the redo/recovery case */
> + if (!gxact->inredo)
> + ereport(ERROR,
> (errcode(ERRCODE_DUPLICATE_OBJECT),
> errmsg("transaction identifier \"%s\" is already in
> use",
> gid)));
> + else
> + {
> + found = true;
> + break;
> + }
> I would not have thought so.
>
> Since we are using the TwoPhaseState structure to track redo entries, at
end of recovery, we will find existing entries. Please see my comments
above for RecoverPreparedTransactions()
> MarkAsPreparing and MarkAsPreparingInRedo really share the same code.
> What if the setup of the dummy PGPROC entry is made conditional?
>
I thought it was cleaner this ways. We can definitely add a bunch of
if-else in MarkAsPreparing() but it won't look pretty.
Regards,
Nikhils
--
Nikhil Sontakke http://www.2ndQuadrant.com/
PostgreSQL/Postgres-XL Development, 24x7 Support, Training & Services
