On Wed, Sep 14, 2011 at 6:21 PM, Kyotaro HORIGUCHI
<horiguchi.kyot...@oss.ntt.co.jp> wrote:
> Hi, This is a review for pg_last_xact_insert_timestamp patch.
> (https://commitfest.postgresql.org/action/patch_view?id=634)
Thanks for the review!
> Q1: The shmem entry for timestamp is not initialized on
> allocating. Is this OK? (I don't know that for OSs other than
> Linux) And zeroing double field is OK for all OSs?
CreateSharedBackendStatus() initializes that shmem entries by doing
MemSet(BackendStatusArray, 0, size). You think this is not enough?
> Nevertheless this is ok for all OSs, I don't know whether
> initializing TimestampTz(double, int64 is ok) field with 8 bytes
> zeros is OK or not, for all platforms. (It is ok for
> IEEE754-binary64).
Which code are you concerned about?
> == Modification detection protocol in pgstat.c
>
> In pgstat_report_xact_end_timestamp, `beentry->st_changecount
> protocol' is used. It is for avoiding reading halfway-updated
> beentry as described in pgstat.h. Meanwhile,
> beentry->st_xact_end_timestamp is not read or (re-)initialized in
> pgstat.c and xlog.c reads only this field of whole beentry and
> st_changecount is not get cared here..
No, st_changecount is used to read st_xact_end_timestamp.
st_xact_end_timestamp is read from the shmem to the local memory
in pgstat_read_current_status(), and this function always checks
st_changecount when reading the shmem value.
> == Code duplication in xact.c
>
> in xact.c, same lines inserted into the end of both IF and ELSE
> blocks.
>
> xact.c:1047> pgstat_report_xact_end_timestamp(xlrec.xact_time);
> xact.c:1073> pgstat_report_xact_end_timestamp(xlrec.xact_time);
>
> These two lines refer to xlrec.xact_time, both of which comes
> from xactStopTimestamp freezed at xact.c:986
>
> xact.c:986> SetCurrentTransactionStopTimestamp();
> xact.c:1008> xlrec.xact_time = xactStopTimestamp;
> xact.c:1051> xlrec.xact_time = xactStopTimestamp;
>
> I think it is better to move this line to just after this ELSE
> block using xactStopTimestamp instead of xlrec.xact_time.
Okay, I've changed the patch in that way.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 13996,14001 **** SELECT set_config('log_statement_stats', 'off', false);
--- 13996,14004 ----
<primary>pg_current_xlog_location</primary>
</indexterm>
<indexterm>
+ <primary>pg_last_xact_insert_timestamp</primary>
+ </indexterm>
+ <indexterm>
<primary>pg_start_backup</primary>
</indexterm>
<indexterm>
***************
*** 14049,14054 **** SELECT set_config('log_statement_stats', 'off', false);
--- 14052,14064 ----
</row>
<row>
<entry>
+ <literal><function>pg_last_xact_insert_timestamp()</function></literal>
+ </entry>
+ <entry><type>timestamp with time zone</type></entry>
+ <entry>Get last transaction log insert time stamp</entry>
+ </row>
+ <row>
+ <entry>
<literal><function>pg_start_backup(<parameter>label</> <type>text</> <optional>, <parameter>fast</> <type>boolean</> </optional>)</function></literal>
</entry>
<entry><type>text</type></entry>
***************
*** 14175,14180 **** postgres=# SELECT * FROM pg_xlogfile_name_offset(pg_stop_backup());
--- 14185,14197 ----
</para>
<para>
+ <function>pg_last_xact_insert_timestamp</> displays the time stamp of last inserted
+ transaction. This is the time at which the commit or abort WAL record for that transaction.
+ If there has been no transaction committed or aborted yet since the server has started,
+ this function returns NULL.
+ </para>
+
+ <para>
For details about proper usage of these functions, see
<xref linkend="continuous-archiving">.
</para>
*** a/doc/src/sgml/high-availability.sgml
--- b/doc/src/sgml/high-availability.sgml
***************
*** 867,872 **** primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
--- 867,881 ----
<command>ps</> command (see <xref linkend="monitoring-ps"> for details).
</para>
<para>
+ You can also calculate the lag in time stamp by comparing the last
+ WAL insert time stamp on the primary with the last WAL replay
+ time stamp on the standby. They can be retrieved using
+ <function>pg_last_xact_insert_timestamp</> on the primary and
+ the <function>pg_last_xact_replay_timestamp</> on the standby,
+ respectively (see <xref linkend="functions-admin-backup-table"> and
+ <xref linkend="functions-recovery-info-table"> for details).
+ </para>
+ <para>
You can retrieve a list of WAL sender processes via the
<link linkend="monitoring-stats-views-table">
<literal>pg_stat_replication</></link> view. Large differences between
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
***************
*** 1066,1071 **** RecordTransactionCommit(void)
--- 1066,1074 ----
(void) XLogInsert(RM_XACT_ID, XLOG_XACT_COMMIT_COMPACT, rdata);
}
+
+ /* Save timestamp of latest transaction commit record */
+ pgstat_report_xact_end_timestamp(xactStopTimestamp);
}
/*
***************
*** 1434,1439 **** RecordTransactionAbort(bool isSubXact)
--- 1437,1445 ----
(void) XLogInsert(RM_XACT_ID, XLOG_XACT_ABORT, rdata);
+ /* Save timestamp of latest transaction abort record */
+ pgstat_report_xact_end_timestamp(xlrec.xact_time);
+
/*
* Report the latest async abort LSN, so that the WAL writer knows to
* flush this abort. There's nothing to be gained by delaying this, since
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 5867,5872 **** pg_is_xlog_replay_paused(PG_FUNCTION_ARGS)
--- 5867,5907 ----
}
/*
+ * Returns timestamp of latest inserted commit/abort record.
+ *
+ * If there has been no transaction committed or aborted yet since
+ * the server has started, this function returns NULL.
+ */
+ Datum
+ pg_last_xact_insert_timestamp(PG_FUNCTION_ARGS)
+ {
+ TimestampTz result = 0;
+ TimestampTz xtime;
+ PgBackendStatus *beentry;
+ int i;
+
+ if (RecoveryInProgress())
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("recovery is in progress"),
+ errhint("WAL control functions cannot be executed during recovery.")));
+
+ beentry = pgstat_fetch_all_beentry();
+
+ for (i = 0; i < MaxBackends; i++, beentry++)
+ {
+ xtime = beentry->st_xact_end_timestamp;
+ if (result < xtime)
+ result = xtime;
+ }
+
+ if (result == 0)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TIMESTAMPTZ(result);
+ }
+
+ /*
* Save timestamp of latest processed commit/abort record.
*
* We keep this in XLogCtl, not a simple static variable, so that it can be
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
***************
*** 254,260 **** static PgStat_StatTabEntry *pgstat_get_tab_entry(PgStat_StatDBEntry *dbentry,
static void pgstat_write_statsfile(bool permanent);
static HTAB *pgstat_read_statsfile(Oid onlydb, bool permanent);
static void backend_read_statsfile(void);
! static void pgstat_read_current_status(void);
static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
static void pgstat_send_funcstats(void);
--- 254,260 ----
static void pgstat_write_statsfile(bool permanent);
static HTAB *pgstat_read_statsfile(Oid onlydb, bool permanent);
static void backend_read_statsfile(void);
! static void pgstat_read_current_status(bool all);
static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
static void pgstat_send_funcstats(void);
***************
*** 2173,2179 **** pgstat_fetch_stat_funcentry(Oid func_id)
PgBackendStatus *
pgstat_fetch_stat_beentry(int beid)
{
! pgstat_read_current_status();
if (beid < 1 || beid > localNumBackends)
return NULL;
--- 2173,2179 ----
PgBackendStatus *
pgstat_fetch_stat_beentry(int beid)
{
! pgstat_read_current_status(false);
if (beid < 1 || beid > localNumBackends)
return NULL;
***************
*** 2192,2202 **** pgstat_fetch_stat_beentry(int beid)
int
pgstat_fetch_stat_numbackends(void)
{
! pgstat_read_current_status();
return localNumBackends;
}
/*
* ---------
* pgstat_fetch_global() -
--- 2192,2220 ----
int
pgstat_fetch_stat_numbackends(void)
{
! pgstat_read_current_status(false);
return localNumBackends;
}
+ /* ----------
+ * pgstat_fetch_all_beentry() -
+ *
+ * Support function for the SQL-callable pgstat* functions. Returns
+ * our local copy of all backend entries.
+ *
+ * NB: caller is responsible for a check if the user is permitted to see
+ * this info (especially the querystring).
+ * ----------
+ */
+ PgBackendStatus *
+ pgstat_fetch_all_beentry(void)
+ {
+ pgstat_read_current_status(true);
+
+ return localBackendStatusTable;
+ }
+
/*
* ---------
* pgstat_fetch_global() -
***************
*** 2414,2419 **** pgstat_bestart(void)
--- 2432,2442 ----
beentry->st_appname[NAMEDATALEN - 1] = '\0';
beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0';
+ /*
+ * Don't reset st_xact_end_timestamp because the previous value can still
+ * be referenced to calculate the latest transaction insert timestamp.
+ */
+
beentry->st_changecount++;
Assert((beentry->st_changecount & 1) == 0);
***************
*** 2560,2565 **** pgstat_report_xact_timestamp(TimestampTz tstamp)
--- 2583,2611 ----
Assert((beentry->st_changecount & 1) == 0);
}
+ /*
+ * Report last transaction end timestamp as the specified value.
+ * Zero means there is no finished transaction.
+ */
+ void
+ pgstat_report_xact_end_timestamp(TimestampTz tstamp)
+ {
+ volatile PgBackendStatus *beentry = MyBEEntry;
+
+ if (!pgstat_track_activities || !beentry)
+ return;
+
+ /*
+ * Update my status entry, following the protocol of bumping
+ * st_changecount before and after. We use a volatile pointer here to
+ * ensure the compiler doesn't try to get cute.
+ */
+ beentry->st_changecount++;
+ beentry->st_xact_end_timestamp = tstamp;
+ beentry->st_changecount++;
+ Assert((beentry->st_changecount & 1) == 0);
+ }
+
/* ----------
* pgstat_report_waiting() -
*
***************
*** 2590,2600 **** pgstat_report_waiting(bool waiting)
* pgstat_read_current_status() -
*
* Copy the current contents of the PgBackendStatus array to local memory,
! * if not already done in this transaction.
* ----------
*/
static void
! pgstat_read_current_status(void)
{
volatile PgBackendStatus *beentry;
PgBackendStatus *localtable;
--- 2636,2647 ----
* pgstat_read_current_status() -
*
* Copy the current contents of the PgBackendStatus array to local memory,
! * if not already done in this transaction. If all is true, the local
! * array includes all entries. Otherwise, it includes only valid ones.
* ----------
*/
static void
! pgstat_read_current_status(bool all)
{
volatile PgBackendStatus *beentry;
PgBackendStatus *localtable;
***************
*** 2636,2642 **** pgstat_read_current_status(void)
int save_changecount = beentry->st_changecount;
localentry->st_procpid = beentry->st_procpid;
! if (localentry->st_procpid > 0)
{
memcpy(localentry, (char *) beentry, sizeof(PgBackendStatus));
--- 2683,2689 ----
int save_changecount = beentry->st_changecount;
localentry->st_procpid = beentry->st_procpid;
! if (localentry->st_procpid > 0 || all)
{
memcpy(localentry, (char *) beentry, sizeof(PgBackendStatus));
***************
*** 2659,2666 **** pgstat_read_current_status(void)
}
beentry++;
! /* Only valid entries get included into the local array */
! if (localentry->st_procpid > 0)
{
localentry++;
localappname += NAMEDATALEN;
--- 2706,2713 ----
}
beentry++;
! /* Only valid entries get included into the local array if all is false */
! if (localentry->st_procpid > 0 || all)
{
localentry++;
localappname += NAMEDATALEN;
*** a/src/include/access/xlog_internal.h
--- b/src/include/access/xlog_internal.h
***************
*** 272,277 **** extern Datum pg_current_xlog_location(PG_FUNCTION_ARGS);
--- 272,278 ----
extern Datum pg_current_xlog_insert_location(PG_FUNCTION_ARGS);
extern Datum pg_last_xlog_receive_location(PG_FUNCTION_ARGS);
extern Datum pg_last_xlog_replay_location(PG_FUNCTION_ARGS);
+ extern Datum pg_last_xact_insert_timestamp(PG_FUNCTION_ARGS);
extern Datum pg_last_xact_replay_timestamp(PG_FUNCTION_ARGS);
extern Datum pg_xlogfile_name_offset(PG_FUNCTION_ARGS);
extern Datum pg_xlogfile_name(PG_FUNCTION_ARGS);
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***************
*** 2869,2874 **** DATA(insert OID = 2850 ( pg_xlogfile_name_offset PGNSP PGUID 12 1 0 0 0 f f f t
--- 2869,2876 ----
DESCR("xlog filename and byte offset, given an xlog location");
DATA(insert OID = 2851 ( pg_xlogfile_name PGNSP PGUID 12 1 0 0 0 f f f t f i 1 0 25 "25" _null_ _null_ _null_ _null_ pg_xlogfile_name _null_ _null_ _null_ ));
DESCR("xlog filename, given an xlog location");
+ DATA(insert OID = 3831 ( pg_last_xact_insert_timestamp PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 1184 "" _null_ _null_ _null_ _null_ pg_last_xact_insert_timestamp _null_ _null_ _null_ ));
+ DESCR("timestamp of last insert xact");
DATA(insert OID = 3810 ( pg_is_in_recovery PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 16 "" _null_ _null_ _null_ _null_ pg_is_in_recovery _null_ _null_ _null_ ));
DESCR("true if server is in recovery");
*** a/src/include/pgstat.h
--- b/src/include/pgstat.h
***************
*** 623,628 **** typedef struct PgBackendStatus
--- 623,631 ----
TimestampTz st_xact_start_timestamp;
TimestampTz st_activity_start_timestamp;
+ /* Time when last transaction ended */
+ TimestampTz st_xact_end_timestamp;
+
/* Database OID, owning user's OID, connection client address */
Oid st_databaseid;
Oid st_userid;
***************
*** 718,723 **** extern void pgstat_bestart(void);
--- 721,727 ----
extern void pgstat_report_activity(const char *cmd_str);
extern void pgstat_report_appname(const char *appname);
extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
+ extern void pgstat_report_xact_end_timestamp(TimestampTz tstamp);
extern void pgstat_report_waiting(bool waiting);
extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
***************
*** 795,800 **** extern void pgstat_send_bgwriter(void);
--- 799,805 ----
extern PgStat_StatDBEntry *pgstat_fetch_stat_dbentry(Oid dbid);
extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry(Oid relid);
extern PgBackendStatus *pgstat_fetch_stat_beentry(int beid);
+ extern PgBackendStatus *pgstat_fetch_all_beentry(void);
extern PgStat_StatFuncEntry *pgstat_fetch_stat_funcentry(Oid funcid);
extern int pgstat_fetch_stat_numbackends(void);
extern PgStat_GlobalStats *pgstat_fetch_global(void);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
