Re: Named restore points

On Tue, Feb 1, 2011 at 10:02 AM, Euler Taveira de Oliveira
<euler(at)timbira(dot)com> wrote:
> Em 14-01-2011 17:41, Jaime Casanova escreveu:
>>
>> Here is a patch that implements "named restore points".
>>
> Sorry, I was swamped with work. :(
>
> Your patch no longer applied so I rebased it and slightly modified it.
> Review is below...
>

Hi,

Thanks for the review, i've been without internet connection for 4
days so i haven't seen the review until now...

> +         The default is to recover to the end of the WAL log.
> +         The precise stopping point is also influenced by
> +         <xref linkend="recovery-target-inclusive">.
> +        </para>
>
> This isn't valid. recovery_target_name are not influenced by
> recovery_target_inclusive. Sentence removed.
>

good point! docs are boring so i was in automatic mode ;)

> + static char recoveryStopNamedRestorePoint[MAXFNAMELEN];
>
> Is MAXFNAMELEN appropriate? AFAICS it is used for file name length. [Looking
> at code...] It seems to be used for backup label too so it is not so
> inappropriate.
>

right, i used it because it is used for backup label

>
> +       else if (recoveryTarget == RECOVERY_TARGET_NAME)
> +               snprintf(buffer, sizeof(buffer),
> +                                "%s%u\t%s\t%s named restore point %s\n",
> +                                (srcfd < 0) ? "" : "\n",
> +                                parentTLI,
> +                                xlogfname,
> +                                recoveryStopAfter ? "after" : "before",
> +                                recoveryStopNamedRestorePoint);
>
> It doesn't matter if it is after or before the restore point. After/Before
> only make sense when we're dealing with transaction or time. Removed.
>

you're right

>                else if (strcmp(item->name, "recovery_target_xid") == 0)
>                {
> +                       /*
> +                        * if recovery_target_name specified, then this
> overrides
> +                        * recovery_target_xid
> +                        */
> +                       if (recoveryTarget == RECOVERY_TARGET_NAME)
> +                               continue;
> +
>
> IMHO the right recovery precedence is xid -> name -> time. If you're
> specifying xid that's because you know what you are doing. Name takes
> precedence over time because it is easier to remember a name than a time. I
> implemented this order in the updated patch.
>

actually i was expecting to hear opinions about this and i agree with you

> +                       recoveryTargetName = pstrdup(item->value);
>
> I also added a check for long names.
>

ok

> +       if ((record->xl_rmid == RM_XLOG_ID) && (record_info ==
> XLOG_RESTORE_POINT))
> +               couldStop = true;
> +
> +       if (!couldStop)
> +               return false;
> +
>
> I reworked this code path because it seems confusing.
>

it is... it was the result of debugging an stupid error on my side...

> +               recordNamedRestorePoint = (xl_named_restore_points *)
> XLogRecGetData(record);
> +               recordXtime = recordNamedRestorePoint->xtime;
>
> Why don't you store the named restore point here too? You will need it a few
> lines below.
>

don't remember, will see

>
> + Datum
> + pg_create_restore_point(PG_FUNCTION_ARGS)
> + {
>
> You should have added a check for long restore point names. Added in the
> updated patch.
>

ok

> +         ereport(NOTICE,
> +                  (errmsg("WAL archiving is not enabled; you must ensure
> that WAL segments are copied through other means for restore points to be
> usefull for you")));
> +
>
> Sentence was rewritten as "WAL archiving is not enabled; you must ensure
> that WAL segments are copied through other means to recover up to named
> restore point".
>

sounds better, thanks

> Finally, this is a nice feature iif we have a way to know what named restore
> points are available. DBAs need to take note of this list (that is not good)
> and the lazy ones will have a hard time to recover the right name (possibly
> with a xlog dump tool).
>
> So how could we store this information? Perhaps a file in
> $PGDATA/pg_xlog/restore_label that contains the label (and possibly the WAL
> location). Also it must have a way to transmit the restore_label when we add
> another restore point. I didn't implement this part (Jaime?) and it seems as
> important as the new xlog record type that is in the patch. It seems
> complicate but I don't have ideas. Anyone? The restore point names could be
> obtained by querying a function (say, pg_restore_point_names or
> pg_restore_point_list).
>

IMHO, probably the best answer is a tool to retrieve that info... the
problem is that a "restore_label" file should be closely attached to
the WAL segment where the named restore point is... and a sql function
won't say anything about named restore points that are in archived WAL
segments...

>
> I will mark this patch waiting on author because of those open issues.
>
> I'm attaching the updated patch and two scripts that I used to play with the
> patch.
>

ok, i will see you're reviewed version later today

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL