Re: [PATCHES] [PgFoundry] Unsigned Data Types [1 of 2]

2008-09-28 Thread Ryan Bradetich
Hello all,

Just wanted to let everyone know I have committed this patch to the
PgFoundry uint project.
I have also updated the commit-fest wiki with this status.

Thanks to everyone (especially Jaime) for the feedback and reviews.

- Ryan

-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches


Re: [PATCHES] [HACKERS] Infrastructure changes for recovery

2008-09-28 Thread Tom Lane
Simon Riggs <[EMAIL PROTECTED]> writes:
>> It does nothing AFAICS for the
>> problem that when restarting archive recovery from a restartpoint,
>> it's not clear when it is safe to start letting in backends.  You need
>> to get past the highest LSN that has made it out to disk, and there is
>> no good way to know what that is.

> AFAICS when we set minRecoveryLoc we *never* unset it. It's recorded in
> the controlfile, so whenever we restart we can see that it has been set
> previously and now we are beyond it.

Right ...

> So if we crash during recovery and
> then restart *after* we reached minRecoveryLoc then we resume in safe
> mode almost immediately.

Wrong.

What minRecoveryLoc is is an upper bound for the LSNs that might be
on-disk in the filesystem backup that an archive recovery starts from.
(Defined as such, it never changes during a restartpoint crash/restart.)
Once you pass that, the on-disk state as modified by any dirty buffers
inside the recovery process represents a consistent database state.
However, the on-disk state alone is not guaranteed consistent.  As you
flush some (not all) of your shared buffers you enter other
not-certainly-consistent on-disk states.  If we crash in such a state,
we know how to use the last restartpoint plus WAL replay to recover to
another state in which disk + dirty buffers are consistent.  However,
we reach such a state only when we have read WAL to beyond the highest
LSN that has reached disk --- and in recovery mode there is no clean
way to determine what that was.

Perhaps a solution is to make XLogFLush not be a no-op in recovery mode,
but have it scribble a highest-LSN somewhere on stable storage (maybe
scribble on pg_control itself, or maybe better someplace else).  I'm
not totally sure about that.  But I am sure that doing nothing will
be unreliable.

regards, tom lane

-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches


Re: [PATCHES] [HACKERS] Infrastructure changes for recovery

2008-09-28 Thread Simon Riggs

On Sun, 2008-09-28 at 14:02 -0400, Tom Lane wrote:

> It does nothing AFAICS for the
> problem that when restarting archive recovery from a restartpoint,
> it's not clear when it is safe to start letting in backends.  You need
> to get past the highest LSN that has made it out to disk, and there is
> no good way to know what that is.
> 
> Unless we can get past this problem the whole thing seems a bit dead
> in
> the water :-(

I agree the importance of your a problem but don't fully understand the
circumstances under which you see a problem arising.

AFAICS when we set minRecoveryLoc we *never* unset it. It's recorded in
the controlfile, so whenever we restart we can see that it has been set
previously and now we are beyond it. So if we crash during recovery and
then restart *after* we reached minRecoveryLoc then we resume in safe
mode almost immediately. If we crash during recovery before we reached
minRecoveryLoc then we continue until we find it. 

There is a loophole, as described on separate post, but that can be
plugged by offering explicit setting of the minRecoveryLoc from
recovery.conf. Most people use pg_start_backup() so do not experience
the need for that.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Training, Services and Support


-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches


Re: [PATCHES] [HACKERS] get_relation_stats_hook()

2008-09-28 Thread Tom Lane
Simon Riggs <[EMAIL PROTECTED]> writes:
> New version of Postgres patch, v5. Implements suggested changes.
> Ready for review and apply.

Applied with some revisions.  The method for passing back freefunc
didn't work, so I made it pass the whole VariableStatsData struct
instead; this might allow some additional flexibility by changing other
fields besides the intended statsTuple and freefunc.  Also, I was still
unhappy about adding a hook in the midst of code that clearly needs
improvement, without making it possible for the hook to override the
adjacent broken code paths; so I refactored the API a bit for that too.

The plugin function would now be something like this:

static bool
plugin_get_relation_stats(PlannerInfo *root,
  RangeTblEntry *rte,
  AttrNumber attnum,
  VariableStatData *vardata)
{
HeapTuplestatstup = NULL;

/* For now, we only cover the simple-relation case */
if (rte->rtekind != RTE_RELATION || rte->inh)
return false;

if (!get_tom_stats_tupletable(rte->relid, attnum))
return false;

/*
 * Get stats if present. We asked for only one row, so no need for loops.
 */
if (SPI_processed > 0)
statstup = SPI_copytuple(SPI_tuptable->vals[0]);

SPI_freetuptable(SPI_tuptable);
SPI_finish();

if (!statstup)
return false;/* should this happen? */

vardata->statsTuple = statstup;
/* define function to use when time to free the tuple */
vardata->freefunc = heap_freetuple;

return true;
}

and if you want to insert stats for expression indexes then there's a
separate get_index_stats_hook for that.

regards, tom lane

-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches


Re: [PATCHES] [HACKERS] Infrastructure changes for recovery

2008-09-28 Thread Tom Lane
Simon Riggs <[EMAIL PROTECTED]> writes:
> On Thu, 2008-09-25 at 18:28 -0400, Tom Lane wrote:
>> After reading this for awhile, I realized that there is a rather
>> fundamental problem with it: it switches into "consistent recovery"
>> mode as soon as it's read WAL beyond ControlFile->minRecoveryPoint.
>> In a crash recovery situation that typically is before the last
>> checkpoint (if indeed it's not still zero), and what that means is
>> that this patch will activate the bgwriter and start letting in
>> backends instantaneously after a crash, long before we can have any
>> certainty that the DB state really is consistent.
>> 
>> In a normal crash recovery situation this would be easily fixed by
>> simply not letting it go to "consistent recovery" state at all, but
>> what about recovery from a restartpoint?  We don't want a slave that's
>> crashed once to never let backends in again.  But I don't see how to
>> determine that we're far enough past the restartpoint to be consistent
>> again.  In crash recovery we assume (without proof ;-)) that we're
>> consistent once we reach the end of valid-looking WAL, but that rule
>> doesn't help for a slave that's following a continuing WAL sequence.
>> 
>> Perhaps something could be done based on noting when we have to pull in
>> a WAL segment from the recovery_command, but it sounds like a pretty
>> fragile assumption.

> Seems like we just say we only signal the postmaster if
> InArchiveRecovery. Archive recovery from a restartpoint is still archive
> recovery, so this shouldn't be a problem in the way you mention. The
> presence of recovery.conf overrides all other cases.

What that implements is my comment that we don't have to let anyone in
at all during a plain crash recovery.  It does nothing AFAICS for the
problem that when restarting archive recovery from a restartpoint,
it's not clear when it is safe to start letting in backends.  You need
to get past the highest LSN that has made it out to disk, and there is
no good way to know what that is.

Unless we can get past this problem the whole thing seems a bit dead in
the water :-(

>> * I'm a bit uncomfortable with the fact that the
>> IsRecoveryProcessingMode flag is read and written with no lock.

> It's not a dynamic state, so I can fix that inside
> IsRecoveryProcessingMode() with a local state to make check faster.

Erm, this code doesn't look like it can allow IsRecoveryProcessingMode
to become locally true in the first place?  I guess you could fix it
by initializing IsRecoveryProcessingMode to true, but that seems likely
to break other places.  Maybe better is to have an additional local
state variable showing whether the flag has ever been fetched from
shared memory.

The other issues don't seem worth arguing about ...

regards, tom lane

-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches