Hi,
On 2014-01-15 02:00:51 -0500, Jaime Casanova wrote:
> On Mon, Nov 18, 2013 at 12:27 PM, Andres Freund <[email protected]>
> wrote:
> > Hi,
> >
> > On 2013-11-15 22:38:05 -0500, Jaime Casanova wrote:
> >> sorry, i clearly misunderstood you. attached a rebased patch with
> >> those functions removed.
> >
> > * --write-standby-enable seems to loose quite some functionality in
> > comparison to --write-recovery-conf since it doesn't seem to set
> > primary_conninfo, standby anymore.
>
> we can add code that looks for postgresql.conf in $PGDATA but if
> postgresql.conf is not there (like the case in debian, there is not
> much we can do about it) or we can write a file ready to be included
> in postgresql.conf, any sugestion?
People might not like me for the suggestion, but I think we should
simply always include a 'recovery.conf' in $PGDATA
unconditionally. That'd make this easier.
Alternatively we could pass a filename to --write-recovery-conf.
> > * CheckRecoveryReadyFile() doesn't seem to be a very descriptive
> > function name.
>
> I left it as CheckStartingAsStandby() but i still have a problem of
> this not being completely clear. this function is useful for standby
> or pitr.
> which leads me to the other problem i have: the recovery trigger file,
> i have left it as standby.enabled but i still don't like it.
> recovery.trigger (Andres objected on this name)
> forced_recovery.trigger
> user_forced_recovery.trigger
stay_in_recovery.trigger? That'd be pretty clear for anybody involved in
pg, but otherwise...
> > * the description of archive_cleanup_command seems wrong to me.
>
> why? it seems to be the same that was in recovery.conf. where did you
> see the description you're complaining at?
I dislike the description in guc.c
> + {"archive_cleanup_command", PGC_POSTMASTER,
> WAL_ARCHIVE_RECOVERY,
> + gettext_noop("Sets the shell command that will be
> executed at every restartpoint."),
> + NULL
> + },
> + &archive_cleanup_command,
previously it was:
> -# specifies an optional shell command to execute at every restartpoint.
> -# This can be useful for cleaning up the archive of a standby server.
> > * Why the PG_TRY/PG_CATCH in check_recovery_target_time? Besides being
> > really strangely formatted (multiline :? inside a function?) it
> > doesn't a) seem to be correct to ignore potential memory allocation
> > errors by just switching back into the context that just errored out,
> > and continue to work there b) forgo cleanup by just continuing as if
> > nothing happened. That's unlikely to be acceptable.
>
> the code that read recovery.conf didn't has that, so i just removed it
Well, that's not necessarily correct. recovery.conf was only read during
startup, while this is read on SIGHUP.
> > * Why do you include xlog_internal.h in guc.c and not xlog.h?
> we actually need both but including xlog_internal.h also includes xlog.h
> i added xlog.h and if someone things is enough only putting
> xlog_internal.h let me know
What's required from xlog_internal.h?
> diff --git a/src/backend/access/transam/xlog.c
> b/src/backend/access/transam/xlog.c
> index b53ae87..54f6a0d 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -64,11 +64,12 @@
> extern uint32 bootstrap_data_checksum_version;
>
> /* File path names (all relative to $PGDATA) */
> -#define RECOVERY_COMMAND_FILE "recovery.conf"
> -#define RECOVERY_COMMAND_DONE "recovery.done"
> +#define RECOVERY_ENABLE_FILE "standby.enabled"
Imo the file and variable names should stay coherent.
> +/* recovery.conf is not supported anymore */
> +#define RECOVERY_COMMAND_FILE "recovery.conf"
> +bool StandbyModeRequested = false;
> +static TimestampTz recoveryDelayUntilTime;
This imo should be lowercase now, the majority of GUC variables are.
> +/* are we currently in standby mode? */
> +bool StandbyMode = false;
Why did you move this?
> - if (rtliGiven)
> + if (strcmp(recovery_target_timeline_string, "") != 0)
> {
Why not have the convention that NULL indicates a unset target_timeline
like you use for other GUCs? Mixing things like this is confusing.
Why is recovery_target_timeline stored as a string? Because it's a
unsigned int? If so, you should have an assign hook setting up a)
rtliGiven, b) properly typed variable.
> - if (rtli)
> + if (recovery_target_timeline)
> {
> /* Timeline 1 does not have a history file, all else
> should */
> - if (rtli != 1 && !existsTimeLineHistory(rtli))
> + if (recovery_target_timeline != 1 &&
> +
> !existsTimeLineHistory(recovery_target_timeline))
> ereport(FATAL,
> (errmsg("recovery target
> timeline %u does not exist",
> - rtli)));
> - recoveryTargetTLI = rtli;
> +
> recovery_target_timeline)));
> + recoveryTargetTLI = recovery_target_timeline;
> recoveryTargetIsLatest = false;
So, now we have a recoveryTargetTLI and recovery_target_timeline
variable? Really? Why do we need recoveryTargetTLI at all now?
> +static void
> +assign_recovery_target_xid(const char *newval, void *extra)
> +{
> + recovery_target_xid = *((TransactionId *) extra);
> +
> + if (recovery_target_xid != InvalidTransactionId)
> + recovery_target = RECOVERY_TARGET_XID;
> + else if (recovery_target_name[0])
> + recovery_target = RECOVERY_TARGET_NAME;
> + else if (recovery_target_time != 0)
> + recovery_target = RECOVERY_TARGET_TIME;
> + else
> + recovery_target = RECOVERY_TARGET_UNSET;
> +}
> +static void
> +assign_recovery_target_time(const char *newval, void *extra)
> +{
> + recovery_target_time = *((TimestampTz *) extra);
> +
> + if (recovery_target_xid != InvalidTransactionId)
> + recovery_target = RECOVERY_TARGET_XID;
> + else if (recovery_target_name[0])
> + recovery_target = RECOVERY_TARGET_NAME;
> + else if (recovery_target_time != 0)
> + recovery_target = RECOVERY_TARGET_TIME;
> + else
> + recovery_target = RECOVERY_TARGET_UNSET;
> +}
> +
I don't think it's correct to do such hangups in the assign hook - you
have no ideas in which order they will be called and such. Imo that
should happen at startup, like we also do it for other interdependent
variables like wal_buffers.
> +static bool
> +check_recovery_target_time(char **newval, void **extra, GucSource source)
> +{
> + TimestampTz time;
> + TimestampTz *myextra;
> +
> + if (strcmp(*newval, "") == 0)
> + time = 0;
> + else
> + time = DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
> +
> CStringGetDatum(*newval),
> +
> ObjectIdGetDatum(InvalidOid),
> +
> Int32GetDatum(-1)));
> +
> + myextra = (TimestampTz *) guc_malloc(ERROR, sizeof(TimestampTz));
> + *myextra = time;
> + *extra = (void *) myextra;
> +
> + return true;
> +}
Trailing space behind the strcmp().
I don't think that's correct. Afaics it will cause the postmaster to
crash on a SIGHUP with invalid data. I think you're unfortunately going
to have to copy a fair bit of timestamptz_in() and even
DateTimeParseError(), replacing the ereport()s by the guc error
reporting.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
