public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/4] bootstrap: convert Typ to a List* 2+ messages / 2 participants [nested] [flat]
* [PATCH 1/4] bootstrap: convert Typ to a List* @ 2020-11-20 02:48 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Justin Pryzby @ 2020-11-20 02:48 UTC (permalink / raw) --- src/backend/bootstrap/bootstrap.c | 89 ++++++++++++++----------------- 1 file changed, 41 insertions(+), 48 deletions(-) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 6f615e6622..1b940d9d27 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -58,7 +58,7 @@ static void BootstrapModeMain(void); static void bootstrap_signals(void); static void ShutdownAuxiliaryProcess(int code, Datum arg); static Form_pg_attribute AllocateAttribute(void); -static void populate_typ_array(void); +static void populate_typ(void); static Oid gettype(char *type); static void cleanup(void); @@ -159,7 +159,7 @@ struct typmap FormData_pg_type am_typ; }; -static struct typmap **Typ = NULL; +static List *Typ = NIL; /* List of struct typmap* */ static struct typmap *Ap = NULL; static Datum values[MAXATTR]; /* current row's attribute values */ @@ -595,10 +595,10 @@ boot_openrel(char *relname) /* * pg_type must be filled before any OPEN command is executed, hence we - * can now populate the Typ array if we haven't yet. + * can now populate Typ if we haven't yet. */ - if (Typ == NULL) - populate_typ_array(); + if (Typ == NIL) + populate_typ(); if (boot_reldesc != NULL) closerel(NULL); @@ -688,7 +688,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness) typeoid = gettype(type); - if (Typ != NULL) + if (Typ != NIL) { attrtypes[attnum]->atttypid = Ap->am_oid; attrtypes[attnum]->attlen = Ap->am_typ.typlen; @@ -866,47 +866,36 @@ cleanup(void) } /* ---------------- - * populate_typ_array + * populate_typ * - * Load the Typ array by reading pg_type. + * Load Typ by reading pg_type. * ---------------- */ static void -populate_typ_array(void) +populate_typ(void) { Relation rel; TableScanDesc scan; HeapTuple tup; - int nalloc; - int i; - - Assert(Typ == NULL); + MemoryContext old; - nalloc = 512; - Typ = (struct typmap **) - MemoryContextAlloc(TopMemoryContext, nalloc * sizeof(struct typmap *)); + Assert(Typ == NIL); rel = table_open(TypeRelationId, NoLock); scan = table_beginscan_catalog(rel, 0, NULL); - i = 0; + old = MemoryContextSwitchTo(TopMemoryContext); while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL) { Form_pg_type typForm = (Form_pg_type) GETSTRUCT(tup); + struct typmap *newtyp; - /* make sure there will be room for a trailing NULL pointer */ - if (i >= nalloc - 1) - { - nalloc *= 2; - Typ = (struct typmap **) - repalloc(Typ, nalloc * sizeof(struct typmap *)); - } - Typ[i] = (struct typmap *) - MemoryContextAlloc(TopMemoryContext, sizeof(struct typmap)); - Typ[i]->am_oid = typForm->oid; - memcpy(&(Typ[i]->am_typ), typForm, sizeof(Typ[i]->am_typ)); - i++; + newtyp = (struct typmap *) palloc(sizeof(struct typmap)); + Typ = lappend(Typ, newtyp); + + newtyp->am_oid = typForm->oid; + memcpy(&newtyp->am_typ, typForm, sizeof(newtyp->am_typ)); } - Typ[i] = NULL; /* Fill trailing NULL pointer */ + MemoryContextSwitchTo(old); table_endscan(scan); table_close(rel, NoLock); } @@ -916,25 +905,26 @@ populate_typ_array(void) * * NB: this is really ugly; it will return an integer index into TypInfo[], * and not an OID at all, until the first reference to a type not known in - * TypInfo[]. At that point it will read and cache pg_type in the Typ array, + * TypInfo[]. At that point it will read and cache pg_type in Typ, * and subsequently return a real OID (and set the global pointer Ap to * point at the found row in Typ). So caller must check whether Typ is - * still NULL to determine what the return value is! + * still NIL to determine what the return value is! * ---------------- */ static Oid gettype(char *type) { - if (Typ != NULL) + if (Typ != NIL) { - struct typmap **app; + ListCell *lc; - for (app = Typ; *app != NULL; app++) + foreach (lc, Typ) { - if (strncmp(NameStr((*app)->am_typ.typname), type, NAMEDATALEN) == 0) + struct typmap *app = lfirst(lc); + if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0) { - Ap = *app; - return (*app)->am_oid; + Ap = app; + return app->am_oid; } } } @@ -949,7 +939,7 @@ gettype(char *type) } /* Not in TypInfo, so we'd better be able to read pg_type now */ elog(DEBUG4, "external type: %s", type); - populate_typ_array(); + populate_typ(); return gettype(type); } elog(ERROR, "unrecognized type \"%s\"", type); @@ -977,17 +967,20 @@ boot_get_type_io_data(Oid typid, Oid *typinput, Oid *typoutput) { - if (Typ != NULL) + if (Typ != NIL) { /* We have the boot-time contents of pg_type, so use it */ - struct typmap **app; - struct typmap *ap; - - app = Typ; - while (*app && (*app)->am_oid != typid) - ++app; - ap = *app; - if (ap == NULL) + struct typmap *ap = NULL; + ListCell *lc; + + foreach (lc, Typ) + { + ap = lfirst(lc); + if (ap->am_oid == typid) + break; + } + + if (!ap || ap->am_oid != typid) elog(ERROR, "type OID %u not found in Typ list", typid); *typlen = ap->am_typ.typlen; -- 2.26.2 --------------BCA1373AE29B32B57608F4EE Content-Type: text/x-patch; charset=UTF-8; name="0002-Allow-composite-types-in-bootstrap-20210307.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-Allow-composite-types-in-bootstrap-20210307.patch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: FDW pushdown of non-collated functions @ 2023-10-05 13:36 Jean-Christophe Arnu <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Jean-Christophe Arnu @ 2023-10-05 13:36 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Dear Hackers, I figured out this email was sent at release time. The worst time to ask for thoughts on a subject IMHO. Anyway, I hope this email will pop the topic over the stack! Thank you! Le ven. 8 sept. 2023 à 16:41, Jean-Christophe Arnu <[email protected]> a écrit : > Dear hackers, > > I recently found a weird behaviour involving FDW (postgres_fdw) and > planning. > > Here’s a simplified use-case: > > Given a remote table (say on server2) with the following definition: > > CREATE TABLE t1( > ts timestamp without time zone, > x bigint, > x2 text > ); > --Then populate t1 table:INSERT INTO t1 > SELECT > current_timestamp - 1000*random()*'1 day'::interval > ,x > ,''||x > FROM > generate_series(1,100000) as x; > > > This table is imported in a specific schema on server1 (we do not use > use_remote_estimate) also with t1 name in a specific schema: > > On server1: > > CREATE SERVER server2 > FOREIGN DATA WRAPPER postgres_fdw > OPTIONS ( > host '127.0.0.1', > port '9002', > dbname 'postgres', > use_remote_estimate 'false' > ); > CREATE USER MAPPING FOR jc > SERVER server2 > OPTIONS (user 'jc'); > CREATE SCHEMA remote; > > IMPORT FOREIGN SCHEMA public > FROM SERVER server2 > INTO remote ; > > On a classic PostgreSQL 15 version the following query using date_trunc() > is executed and results in the following plan: > > jc=# explain (verbose,analyze) select date_trunc('day',ts), count(1) from remote.t1 group by date_trunc('day',ts) order by 1; > QUERY PLAN ----------------------------------------------------------------------------------------------------------------------------------- > Sort (cost=216.14..216.64 rows=200 width=16) (actual time=116.699..116.727 rows=1001 loops=1) > Output: (date_trunc('day'::text, ts)), (count(1)) > Sort Key: (date_trunc('day'::text, t1.ts)) > Sort Method: quicksort Memory: 79kB > -> HashAggregate (cost=206.00..208.50 rows=200 width=16) (actual time=116.452..116.532 rows=1001 loops=1) > Output: (date_trunc('day'::text, ts)), count(1) > Group Key: date_trunc('day'::text, t1.ts) > Batches: 1 Memory Usage: 209kB > -> Foreign Scan on remote.t1 (cost=100.00..193.20 rows=2560 width=8) (actual time=0.384..106.225 rows=100000 loops=1) > Output: date_trunc('day'::text, ts) > Remote SQL: SELECT ts FROM public.t1 > Planning Time: 0.077 ms > Execution Time: 117.028 ms > > > Whereas the same query with date_bin() > > jc=# explain (verbose,analyze) select date_bin('1day',ts,'2023-01-01'), count(1) from remote.t1 group by 1 order by 1; > QUERY PLAN ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > Foreign Scan (cost=113.44..164.17 rows=200 width=16) (actual time=11.297..16.312 rows=1001 loops=1) > Output: (date_bin('1 day'::interval, ts, '2023-01-01 00:00:00'::timestamp without time zone)), (count(1)) > Relations: Aggregate on (remote.t1) > Remote SQL: SELECT date_bin('1 day'::interval, ts, '2023-01-01 00:00:00'::timestamp without time zone), count(1) FROM public.t1 GROUP BY 1 ORDER BY date_bin('1 day'::interval, ts, '2023-01-01 00:00:00'::timestamp without time zone) ASC NULLS LAST > Planning Time: 0.114 ms > Execution Time: 16.599 ms > > > > With date_bin() the whole expression is pushed down to the remote server, > whereas with date_trunc() it’s not. > > I dived into the code and live debugged. It turns out that decisions to > pushdown or not a whole query depends on many factors like volatility and > collation. In the date_trunc() case, the problem is all about collation ( > date_trunc() on timestamp without time zone). And decision is made in the > foreign_expr_walker() in deparse.c ( > https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=contrib/postgres_fdw/deparse.c;h=efaf38... > ) > > First the function is tested as shippable (able to be pushed down) and > date_trunc() and date_bin() both are. > > Then parameters sub-expressions are evaluated with collation and > “shippability”, and they all are with both functions. > > Then we arrive at this code portion: > > if (fe->inputcollid == InvalidOid) > /* OK, inputs are all noncollatable */ ;else if (inner_cxt.state != FDW_COLLATE_SAFE || > fe->inputcollid != inner_cxt.collation) > return false; > > For date_trunc() function : > > - > > fe variable contains the sub-expressions/arguments merged constraints > such as fe->inputcollid. This field is evaluated to 100 (default > collation) so codes jumps to else statement and evaluates the if > predicates. This 100 inputcollationid is due to text predicate 'day'. > - > > inner_cxt.state contains FDW_COLLATE_STATE but inner_cxt.collation > contains 0 (InvalidOid) so the control flow returns false thus the > function cannot be pushed down. > > For date_bin() function : > > - fe variable contains the sub-expressions/arguments merged > constraints. Here, fe->inputcollid is evaluated to 0 (InvalidOid) thus > skips the else statement and continues the control flow in the > function. > > For date_bin(), all arguments are “non-collatable” arguments (timestamp > without time zone and interval). > > So the situation is that date_trunc() is a “non-collatable” function > failing to be pushed down whereas it may be a good idea to do so. > > Maybe we could add another condition to the first if statement in order to > allow a “no-collation” function to be pushed down even if they have > “collatable” parameters. I’m not sure about the possible regressions of > behaviour of this change, but it seems to work fine with date_trunc() and > date_part() (which suffers the same problem). > > Here’s the following change > > /* > * If function's input collation is not derived from a foreign > * Var, it can't be sent to remote. > */if (fe->inputcollid == InvalidOid || > fe->funccollid == InvalidOid) > /* OK, inputs are all noncollatable */ ;else if (inner_cxt.state != FDW_COLLATE_SAFE || > fe->inputcollid != inner_cxt.collation) > return false; > > I don’t presume this patch is free from side effects or fits all use-cases. > > A patch (tiny) is attached to this email. This patch works against > master/head at the time of writing. > Thank you for any thoughts. > > -- > Jean-Christophe Arnu > -- Jean-Christophe Arnu ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-10-05 13:36 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 02:48 [PATCH 1/4] bootstrap: convert Typ to a List* Justin Pryzby <[email protected]> 2023-10-05 13:36 Re: FDW pushdown of non-collated functions Jean-Christophe Arnu <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox