diff --git a/src/spock.c b/src/spock.c index 117c3e25..16c9cb86 100644 --- a/src/spock.c +++ b/src/spock.c @@ -194,6 +194,8 @@ static PGconn *spock_connect_base(const char *connstr, const char *appname, const char *suffix, bool replication); +static char *spock_get_conninfo_option(const char *connstr, + const char *keyword); /* * Ensure string is not longer than maxlen. @@ -351,7 +353,12 @@ get_spock_table_oid(const char *table) return reloid; } -#define CONN_PARAM_ARRAY_SIZE 10 +/* + * One more than the number of parameters we pass, to leave room for the NULL + * terminator. In passing, bumped from 10 when "options" was added; the array + * was previously an exact fit, which left no margin for the next parameter. + */ +#define CONN_PARAM_ARRAY_SIZE 11 static PGconn * spock_connect_base(const char *connstr, const char *appname, @@ -362,6 +369,8 @@ spock_connect_base(const char *connstr, const char *appname, const char *keys[CONN_PARAM_ARRAY_SIZE]; const char *vals[CONN_PARAM_ARRAY_SIZE]; char appname_buf[NAMEDATALEN]; + char *user_options; + char *options_val; StringInfoData s; initStringInfo(&s); @@ -402,16 +411,44 @@ spock_connect_base(const char *connstr, const char *appname, keys[i] = "replication"; vals[i] = replication ? "database" : NULL; i++; + + /* + * Force assorted GUC parameters to settings that ensure that the peer will + * output data values in a form that is unambiguous to us when the transfer + * happens in text mode. (We don't touch our own settings; that would + * surprise user-defined code running here, such as triggers.) ISO is the + * only DateStyle that always emits an unambiguous date and an explicit + * numeric UTC offset, which is also why TimeZone needs no forcing. + * + * Subscription sync reads spock.progress from a peer over a regular + * connection, so this is applied to those as well, not only to replication + * ones. + * + * Caution: an explicit array entry overrides any "options" carried by the + * expanded dbname string, so the user's must be prepended rather than + * replaced. Ours win by coming last, as the backend applies -c in order. + */ + user_options = spock_get_conninfo_option(connstr, "options"); + options_val = psprintf("%s -c datestyle=ISO -c intervalstyle=postgres -c extra_float_digits=3", + (user_options == NULL) ? "" : user_options); + if (user_options != NULL) + pfree(user_options); + + keys[i] = "options"; + vals[i] = options_val; + i++; + keys[i] = NULL; vals[i] = NULL; - Assert(i <= CONN_PARAM_ARRAY_SIZE); + Assert(i < CONN_PARAM_ARRAY_SIZE); /* * We use the expand_dbname parameter to process the connection string (or * URI), and pass some extra options. */ conn = PQconnectdbParams(keys, vals, /* expand_dbname = */ true); + if (PQstatus(conn) != CONNECTION_OK) { ereport(ERROR, @@ -421,11 +458,73 @@ spock_connect_base(const char *connstr, const char *appname, errdetail("dsn was: %s", s.data))); } + /* + * Released only once nothing can read vals[] again. libpq copied what it + * needed during the call above, so freeing earlier would work, but it would + * leave a dangling entry in vals[] across the error path -- and that path + * already formats connection details for the report. + */ + pfree(options_val); + resetStringInfo(&s); return conn; } +/* + * spock_get_conninfo_option + * Return the value of a single option from a connection string, or NULL + * if it is absent or empty. + * + * Result is palloc'd in CurrentMemoryContext. If the option appears more + * than once the last occurrence wins, matching libpq's own precedence. + * + * Modelled on libpqrcv_get_option_from_conninfo() in + * src/backend/replication/libpqwalreceiver/libpqwalreceiver.c, but + * deliberately does not raise an error on an unparseable string; see below. + */ +static char * +spock_get_conninfo_option(const char *connstr, const char *keyword) +{ + PQconninfoOption *opts; + PQconninfoOption *opt; + char *option = NULL; + char *err = NULL; + + Assert(connstr != NULL); + Assert(keyword != NULL); + + opts = PQconninfoParse(connstr, &err); + if (opts == NULL) + { + /* + * Deliberately not an error. PQconnectdbParams() takes a dbname with + * neither '=' nor a URI prefix literally, so a DSN of plain "mydb" + * connects today while PQconninfoParse() rejects it; raising an error + * here would break such configurations. Report no options instead, and + * let PQconnectdbParams() complain if the string really is malformed. + * + * The error string is malloc'd, so we must free it explicitly. + */ + PQfreemem(err); + return NULL; + } + + for (opt = opts; opt->keyword != NULL; opt++) + { + if (strcmp(opt->keyword, keyword) == 0 && opt->val && opt->val[0] != '\0') + { + if (option != NULL) + pfree(option); + option = pstrdup(opt->val); + } + } + + PQconninfoFree(opts); + + return option; +} + /* * Make standard postgres connection, ERROR on failure. diff --git a/src/spock_common.c b/src/spock_common.c index 0c0cd8f3..3f4ec25a 100644 --- a/src/spock_common.c +++ b/src/spock_common.c @@ -21,6 +21,7 @@ #include "storage/lmgr.h" #include "storage/proc.h" #include "utils/builtins.h" +#include "utils/datetime.h" #include "utils/guc.h" #include "utils/lsyscache.h" #include "utils/memutils.h" @@ -668,13 +669,128 @@ write_buf(int fd, const void *buf, size_t nbytes, const char *filename) } } +/* + * commit_ts_str_has_offset + * Does this commit timestamp text representation carry an explicit UTC + * offset? + * + * Uses ParseDateTime(), the same tokeniser timestamptz_in() runs, so that + * this predicate cannot drift away from what the parser actually accepts. + * Hand-rolled scanning was tried first and got the era suffix wrong: ISO + * output for a BC date is "0044-03-15 11:45:16-00:14:44 BC", where the + * offset is not the last field. + * + * Only the presence of an offset is decided here. Whether the value + * denotes a real instant is a separate question, and the caller answers it + * separately; do not fold the two together. + */ +static bool +commit_ts_str_has_offset(const char *s) +{ + char workbuf[MAXDATELEN + MAXDATEFIELDS]; + char *field[MAXDATEFIELDS]; + int ftype[MAXDATEFIELDS]; + int nf; + int i; + + Assert(s != NULL); + + /* + * Not tokenisable as a datetime at all. The caller parses before asking, so + * a string that reaches us has already satisfied timestamptz_in() and this + * should not happen; answer no rather than claim an offset we did not see. + * Note the contract is that negative means failure, zero or positive + * success. + */ + if (ParseDateTime(s, workbuf, sizeof(workbuf), field, ftype, + MAXDATEFIELDS, &nf) < 0) + return false; + + /* + * DTK_TZ is a numeric UTC offset. A zone abbreviation arrives as + * DTK_STRING instead and is deliberately not accepted here: an abbreviation + * is not unique across zones -- CST is China, Cuba and US Central -- so it + * does not pin down an instant even when the reader can resolve it. + */ + for (i = 0; i < nf; i++) + { + if (ftype[i] == DTK_TZ) + return true; + } + + return false; +} + +/* + * str_to_timestamptz + * Parse a commit timestamp that reached us from another node. + * + * Deliberately not a general timestamptz parser. Its only inputs are + * spock.progress.remote_commit_ts and .last_updated_ts, read from a peer + * over libpq, and both are instants stamped by a running server. A finite + * value bearing an explicit offset is the only thing that can legitimately + * arrive, so everything else is refused rather than interpreted. + * + * Peer sessions are started with datestyle=ISO (see spock_connect_base()), + * so the offset should always be there. If it is not, something between us + * and the peer has changed the representation; timestamptz_in() would then + * quietly resolve the value in the local TimeZone and move the instant, so + * refuse rather than guess. + * + * Both checks should be unreachable in a correctly configured cluster. + * They are ereport() and not Assert() on purpose: this string arrives over + * the network, which makes it input to be validated rather than an internal + * invariant, and an Assert would be compiled out of exactly the production + * builds where a mangled value does its damage. + * + * Note the order. Parsing comes first so that a malformed string draws + * timestamptz_in()'s own complaint, which is better than anything we could + * phrase, and so that "infinity" is diagnosed as the non-instant it is + * rather than as a missing time zone. Checking the offset first looked + * tidier but made the finiteness test unreachable: no spelling of infinity + * carries an offset, so it was always rejected one branch earlier, under a + * message that would have sent the reader hunting through the peer's + * DateStyle for nothing. Parsing a zone-less value here is harmless; the + * result is discarded before anyone can use it. + */ TimestampTz str_to_timestamptz(const char *s) { - return DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in, - CStringGetDatum(s), - ObjectIdGetDatum(InvalidOid), - Int32GetDatum(-1))); + TimestampTz result; + + result = DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in, + CStringGetDatum(s), + ObjectIdGetDatum(InvalidOid), + Int32GetDatum(-1))); + + /* + * Rejects infinity and -infinity, which no server stamps a commit with. + * Letting one through would put INT64_MAX or INT64_MIN into + * SpockApplyProgress and from there into spock.progress, where every lag + * figure and every prev_remote_ts comparison derived from it turns into + * nonsense. spock_sync.c already Asserts IS_VALID_TIMESTAMP on the result; + * this makes the same check hold in production builds, at the one place all + * such values pass through. + */ + if (!IS_VALID_TIMESTAMP(result)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_DATETIME_FORMAT), + errmsg("commit timestamp \"%s\" received from a peer node is not a finite instant", + s))); + + if (!commit_ts_str_has_offset(s)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_DATETIME_FORMAT), + errmsg("commit timestamp \"%s\" received from a peer node carries no time zone", + s), + errdetail("A commit timestamp denotes an absolute instant; " + "interpreting this one in the local time zone would " + "shift it."), + errhint("The peer session is expected to run with " + "datestyle=ISO, which Spock requests when it " + "connects."))); + + return result; } XLogRecPtr diff --git a/tests/tap/schedule b/tests/tap/schedule index 88f3e757..f73dcf8d 100644 --- a/tests/tap/schedule +++ b/tests/tap/schedule @@ -54,6 +54,7 @@ test: 030_autoddl_repset_stickiness test: 032_lolor_largeobject_repset test: 033_zodan_lolor_add_node test: 034_reserved_object_ddl +test: 037_wire_format_datestyle test: 044_apply_change_logging test: 045_lsn_from_commit_ts # Upgrade schema match test (builds from source, slow): diff --git a/tests/tap/t/037_wire_format_datestyle.pl b/tests/tap/t/037_wire_format_datestyle.pl new file mode 100644 index 00000000..4af6c647 --- /dev/null +++ b/tests/tap/t/037_wire_format_datestyle.pl @@ -0,0 +1,268 @@ +#!/usr/bin/perl +# ============================================================================= +# Test: 037_wire_format_datestyle.pl - Peer sessions must use a canonical +# value representation on the wire +# ============================================================================= +# Spock moves values between nodes as TEXT in two places: +# +# 1. The native protocol falls back to text transfer whenever the binary +# representation is unavailable -- when the two nodes disagree on major +# version, or when the subscription sets force_text_transfer. User column +# values are then rendered by the walsender with OidOutputFunctionCall() +# and parsed by the apply worker with OidInputFunctionCall(). +# 2. spock.progress.remote_commit_ts is read from a peer over libpq during +# subscription sync (adjust_progress_info(), +# spock_create_slot_and_read_progress()) and parsed with +# str_to_timestamptz(). +# +# Nothing in either protocol negotiates the text format, so it follows whatever +# DateStyle / TimeZone each end happens to run with. Under DateStyle 'SQL' or +# 'German' the date component is ordered by the sender's MDY/DMY setting and the +# zone appears as an abbreviation rather than a numeric offset. A receiver with +# the opposite setting reads a DIFFERENT DAY, and raises nothing: 05/06/2025 +# rendered MDY and read DMY is a silent 30-day error. +# +# spock_connect_base() therefore passes +# -c datestyle=ISO -c intervalstyle=postgres -c extra_float_digits=3 +# in the startup packet of every peer connection, mirroring what +# libpqrcv_connect() does for core logical replication. ISO is the only style +# that always emits an unambiguous date and an explicit numeric UTC offset. This +# test asserts that the pinning actually holds. +# +# Note TimeZone is deliberately NOT forced, and this test covers that decision: +# the nodes are left in different zones on purpose. Under ISO a timestamptz +# always carries its offset, so the receiver reconstructs the same instant +# whatever zone either end runs in. +# +# Setup: the two nodes are configured to disagree as sharply as possible. +# n1 = provider DateStyle 'SQL, MDY', TimeZone 'Asia/Tokyo' +# n2 = subscriber DateStyle 'SQL, DMY', TimeZone 'UTC' +# The subscription uses force_text_transfer := true so the text path is taken +# deterministically, without needing two PostgreSQL major versions. +# +# The test data deliberately uses day-of-month <= 12 so that BOTH the MDY and +# the DMY reading are valid calendar dates. That is what makes the failure +# silent rather than an error, and it is the case a test must cover: an +# out-of-range day would merely raise, which is easy to notice. +# +# Without the fix the replicated timestamps land on the wrong day. With it they +# match the source exactly. +# ============================================================================= + +use strict; +use warnings; +use Test::More; +use lib '.'; +use SpockTest qw(create_cluster destroy_cluster system_or_bail + get_test_config scalar_query psql_or_bail + wait_for_sub_status wait_for_pg_ready); + +# PGTZ/PGDATESTYLE in the environment would override the server settings for +# our own psql sessions and mask what we are trying to measure. +delete $ENV{PGTZ}; +delete $ENV{PGDATESTYLE}; + +my $DS_PROVIDER = 'SQL, MDY'; +my $TZ_PROVIDER = 'Asia/Tokyo'; +my $DS_SUBSCRIBER = 'SQL, DMY'; +my $TZ_SUBSCRIBER = 'UTC'; + +create_cluster(2, 'Create 2-node cluster'); + +my $config = get_test_config(); +my $node_ports = $config->{node_ports}; +my $dbname = $config->{db_name}; +my $host = $config->{host}; +my $pg_bin = $config->{pg_bin}; + +my $conn_n1 = "host=$host port=$node_ports->[0] dbname=$dbname"; + +# ============================================================================= +# SETUP: make the two nodes disagree about DateStyle and TimeZone +# ============================================================================= +# These must be server settings, not per-session ones: the sessions that do the +# rendering are the walsender on n1 and the apply worker on n2, neither of which +# we can reach with a SET. + +psql_or_bail(1, "ALTER SYSTEM SET DateStyle = '$DS_PROVIDER'"); +psql_or_bail(1, "ALTER SYSTEM SET TimeZone = '$TZ_PROVIDER'"); +psql_or_bail(2, "ALTER SYSTEM SET DateStyle = '$DS_SUBSCRIBER'"); +psql_or_bail(2, "ALTER SYSTEM SET TimeZone = '$TZ_SUBSCRIBER'"); + +for my $i (0, 1) { + system_or_bail "$pg_bin/psql", '-X', '-p', $node_ports->[$i], '-d', $dbname, + '-c', 'SELECT pg_reload_conf()'; +} + +ok(wait_for_pg_ready($host, $node_ports->[0], $pg_bin, 30), 'n1 ready after reload'); +ok(wait_for_pg_ready($host, $node_ports->[1], $pg_bin, 30), 'n2 ready after reload'); + +# scalar_query strips whitespace, so compare against the squeezed spelling. +(my $ds_provider_squeezed = $DS_PROVIDER) =~ s/\s+//g; +(my $ds_subscriber_squeezed = $DS_SUBSCRIBER) =~ s/\s+//g; + +is(scalar_query(1, 'SHOW DateStyle'), $ds_provider_squeezed, + "n1 DateStyle is $DS_PROVIDER"); +is(scalar_query(2, 'SHOW DateStyle'), $ds_subscriber_squeezed, + "n2 DateStyle is $DS_SUBSCRIBER"); +is(scalar_query(1, 'SHOW TimeZone'), $TZ_PROVIDER, "n1 TimeZone is $TZ_PROVIDER"); +is(scalar_query(2, 'SHOW TimeZone'), $TZ_SUBSCRIBER, "n2 TimeZone is $TZ_SUBSCRIBER"); + +# ============================================================================= +# NEGATIVE CONTROL: prove the hazard is live in this configuration +# ============================================================================= +# Without this, a green run proves nothing: if the assertions further down passed +# because the two nodes happened to agree, or because the values never went +# through text at all, the test would be reporting success for the wrong reason. +# +# So first demonstrate that a date rendered on n1 and read back on n2 really does +# change meaning. Only the date component is used, deliberately: it isolates +# DateStyle and needs no zone abbreviation to be resolvable on either node. + +my $render_n1 = scalar_query(1, "SELECT '2025-05-06 10:30:45+00'::timestamptz::text"); +my $render_n2 = scalar_query(2, "SELECT '2025-05-06 10:30:45+00'::timestamptz::text"); +isnt($render_n1, $render_n2, + "The nodes render the same instant differently (n1: $render_n1, n2: $render_n2)"); + +# n1 renders 6 May 2025 in its own DateStyle; 'SQL, MDY' gives "05/06/2025". +my $date_on_wire = scalar_query(1, "SELECT '2025-05-06'::date::text"); +is($date_on_wire, '05/06/2025', + "n1 renders 2025-05-06 as $date_on_wire"); + +# n2 reads that same text under 'SQL, DMY' and lands on 5 June: 30 days out, with +# no error, because both readings are real dates. +my $drift = scalar_query(2, + "SELECT '$date_on_wire'::date - '2025-05-06'::date"); +is($drift, '30', + "n2 reads $date_on_wire as a date 30 days away -- the misinterpretation this " + . "test exists to catch is reachable here"); + +# ============================================================================= +# SETUP: a table covering every datetime type that goes through the text path +# ============================================================================= + +my $ddl = 'CREATE TABLE wire_fmt (' + . 'id int primary key, ' + . 'ts timestamptz, ' # instant; the zone must survive + . 'tsn timestamp, ' # wall clock; the date order must survive + . 'd date, ' # date order only + . 'iv interval, ' # IntervalStyle + . 'f float8)'; # extra_float_digits + +psql_or_bail(1, "SELECT spock.repset_create('wireset')"); +psql_or_bail(2, "SELECT spock.repset_create('wireset')"); +psql_or_bail(1, $ddl); +psql_or_bail(2, $ddl); +psql_or_bail(1, "SELECT spock.repset_add_table('wireset', 'wire_fmt')"); +psql_or_bail(2, "SELECT spock.repset_add_table('wireset', 'wire_fmt')"); +pass('Created replication set and test table on both nodes'); + +# force_text_transfer := true takes the text path deterministically, so the test +# does not need two PostgreSQL major versions to reach it. +psql_or_bail(2, "SELECT spock.sub_create( + subscription_name := 'sub_n2_n1', + provider_dsn := '$conn_n1', + replication_sets := ARRAY['wireset'], + synchronize_structure := false, + synchronize_data := false, + force_text_transfer := true +)"); +ok(wait_for_sub_status(2, 'sub_n2_n1', 'replicating', 60), + 'Subscription n2->n1 is replicating with force_text_transfer'); + +# Confirm the flag was actually recorded, not silently dropped by sub_create(). +# That matters more than it looks: the flag is what makes the text path certain. +# spock_start_replication() sends binary.want_internal_basetypes and +# binary.want_binary_basetypes as '0' when it is set, so the publisher leaves +# allow_internal_basetypes and allow_binary_basetypes at false, and +# decide_datum_transfer() has no branch left but 't'. There is no negotiation +# that could quietly restore binary transfer behind our back, so if the flag is +# set, every value below travelled as text. +is(scalar_query(2, + "SELECT sub_force_text_transfer FROM spock.subscription + WHERE sub_name = 'sub_n2_n1'"), + 't', + 'force_text_transfer is recorded on the subscription, so transfer is text'); + +# ============================================================================= +# Replicate values whose MDY and DMY readings are both valid dates +# ============================================================================= +# Day <= 12 in every row, so a swapped date order produces another real date +# instead of an error. +# +# Mind the provider's zone when editing these. A timestamptz is rendered in the +# provider's TimeZone, so the day-of-month that goes on the wire is the one in +# Asia/Tokyo, not in UTC. An earlier revision used 2025-01-12 23:59:59+00 here, +# which is 2025-01-13 in Tokyo: the swapped reading became month 13, apply raised +# "date/time field value out of range", and the subscription disabled itself. The +# test still failed without the fix, but for the wrong reason -- it stopped +# demonstrating the quiet misreading it is supposed to be about. Every instant +# below is chosen so that its Tokyo date also has a day of month not above 12. + +psql_or_bail(1, "INSERT INTO wire_fmt VALUES + (1, '2025-05-06 10:30:45.123456+00', '2025-05-06 10:30:45.123456', + '2025-05-06', '1 day 02:03:04', 1.2345678901234567), + (2, '2025-01-11 23:59:59.999999+00', '2025-01-11 23:59:59.999999', + '2025-01-11', '-3 days', 0.1), + (3, '2025-11-02 01:30:00+00', '2025-11-02 01:30:00', + '2025-11-02', '00:00:01', -9.87654321e10)"); + +my $applied = 0; +for my $attempt (1 .. 60) { + $applied = scalar_query(2, 'SELECT count(*) FROM wire_fmt'); + last if defined $applied && $applied eq '3'; + sleep 1; +} +is($applied, '3', 'All three rows reached n2 over the text path'); + +# ============================================================================= +# ASSERTIONS: compare instants, not rendered strings +# ============================================================================= +# Both sides are asked for epoch seconds, which carry no zone and no date order, +# so the comparison is immune to the very settings under test. Any surviving +# difference is a real difference in the stored value. + +for my $id (1 .. 3) { + my $src = scalar_query(1, + "SELECT extract(epoch FROM ts)::numeric::text FROM wire_fmt WHERE id = $id"); + my $dst = scalar_query(2, + "SELECT extract(epoch FROM ts)::numeric::text FROM wire_fmt WHERE id = $id"); + is($dst, $src, "row $id: timestamptz replicated as the same instant ($src)"); + + my $src_n = scalar_query(1, + "SELECT extract(epoch FROM tsn)::numeric::text FROM wire_fmt WHERE id = $id"); + my $dst_n = scalar_query(2, + "SELECT extract(epoch FROM tsn)::numeric::text FROM wire_fmt WHERE id = $id"); + is($dst_n, $src_n, "row $id: timestamp replicated with the same date"); + + my $src_d = scalar_query(1, + "SELECT to_char(d, 'YYYY-MM-DD') FROM wire_fmt WHERE id = $id"); + my $dst_d = scalar_query(2, + "SELECT to_char(d, 'YYYY-MM-DD') FROM wire_fmt WHERE id = $id"); + is($dst_d, $src_d, "row $id: date replicated unswapped ($src_d)"); + + my $src_i = scalar_query(1, + "SELECT extract(epoch FROM iv)::numeric::text FROM wire_fmt WHERE id = $id"); + my $dst_i = scalar_query(2, + "SELECT extract(epoch FROM iv)::numeric::text FROM wire_fmt WHERE id = $id"); + is($dst_i, $src_i, "row $id: interval replicated intact"); + + my $src_f = scalar_query(1, "SELECT f::text FROM wire_fmt WHERE id = $id"); + my $dst_f = scalar_query(2, "SELECT f::text FROM wire_fmt WHERE id = $id"); + is($dst_f, $src_f, "row $id: float8 replicated without losing digits"); +} + +# A whole-table checksum over canonically formatted values: catches anything the +# per-column assertions above might have missed. +my $sum_sql = + "SELECT md5(string_agg( + id || '|' || extract(epoch FROM ts) || '|' || + extract(epoch FROM tsn) || '|' || to_char(d, 'YYYY-MM-DD') || '|' || + extract(epoch FROM iv) || '|' || f, ',' ORDER BY id)) + FROM wire_fmt"; +is(scalar_query(2, $sum_sql), scalar_query(1, $sum_sql), + 'Whole table matches between provider and subscriber'); + +destroy_cluster('Destroy cluster'); + +done_testing();