Re: Re: [HACKERS] [PROPOSAL] Temporal query processing with range types

On Tue, Mar 5, 2019 at 3:46 AM David Steele <david(at)pgmasters(dot)net> wrote:
> I have marked this entry as targeting PG13 since it is too late to
> consider for PG12.

Sounds right. As Peter said himself, this patch is WIP, so it's too
soon to consider integrating it. This is also fairly evident from the
content of the patch, which is full of comments marked XXX and PEMOSER
that obviously need to be addressed somehow. For all of that, I'd say
this patch is much closer to being on the right track than the old
design, even though it's clear that a lot of work remains.

Some desultory review comments:

+#define setSweepline(datum) \
+ node->sweepline = datumCopy(datum, node->datumFormat->attbyval,
node->datumFormat->attlen)
+
+#define freeSweepline() \
+ if (! node->datumFormat->attbyval) pfree(DatumGetPointer(node->sweepline))

I am quite dubious about this. Almost everywhere in the executor we
rely on slots to keep track of tuples and free memory for us. It seems
unlikely that this should be the one place where we have code that
looks completely different. Aside from that, this seems to propose
there is only one relevant column, which seems like an assumption that
we probably don't want to bake too deeply into the code.

+ ereport(ERROR,
+ (errcode(ERRCODE_NOT_NULL_VIOLATION),
+ errmsg("Attribute \"%s\" at position %d is null. Temporal " \
+ "adjustment not possible.",
+ NameStr(TupleDescAttr(slot->tts_tupleDescriptor, attnum - 1)->attname),
+ attnum)));

This error message is marked as translatable (ereport used rather than
elog) but the error-message text is unsuitable for a user-facing
error. If this is a user-facing error, we need a better error, or
maybe we need to rethink the semantics altogether (e.g. just skip such
rows instead of erroring out, or something). If it's an internal
error that should not be possible no matter what query the user
enters, and is only here as a sanity test, just simplify and use elog
(and maybe add some comments explaining why that's so).

+ * heapGetAttrNotNull

I may be a bit behind the times here, but it seems to me that this is
functionally equivalent to slotGetAttrNotNull and thus we shouldn't
need both.

+ bool empty = false;

Not much point in declaring a variable whose value is never changed
and whose value takes up exactly the same number of characters as the
variable name.

+ * temporalAdjustmentStoreTuple
+ * While we store result tuples, we must add the newly calculated temporal
+ * boundaries as two scalar fields or create a single range-typed field
+ * with the two given boundaries.

This doesn't seem to describe what the function is actually doing.

+ * This should ideally be done with RangeBound types on the right-hand-side
+ * created during range_split execution. Otherwise, we loose information about
+ * inclusive/exclusive bounds and infinity. We would need to implement btree
+ * operators for RangeBounds.

This seems like an idea for future improvement, but it's unclear to me
how the proposed idea is different from the state created by the
patch.

Also, materializing the slot to a heap tuple so that we can modify it
seems inefficient. I wonder if we can't use a virtual slot here.

+ if (qual->opno == OID_RANGE_EQ_OP) {
+ Oid rngtypid;
+
+ // XXX PEMOSER Change opfamily and opfunc
+ qual->opfuncid = F_RANGE_CONTAINS; //<<--- opfuncid can be 0 during planning
+ qual->opno = OID_RANGE_CONTAINS_ELEM_OP; //OID_RANGE_CONTAINS_OP;
+ clause->isnormalize = true;
+
+ // Attention: cannot merge using non-equality operator 3890 <---
OID_RANGE_CONTAINS_OP
+ opfamily = 4103; //range_inclusion_ops from pg_opfamily.h
+
+ rngtypid = exprType((Node*)clause->lexpr->expr);
+ clause->range_typcache = lookup_type_cache(rngtypid, TYPECACHE_RANGE_INFO);
+ testmytypcache = clause->range_typcache;
+ } else {
+ clause->isnormalize = false;
+ }

This is pretty obviously a mess of hardcoded constants which are,
furthermore, not explained. I can't tell whether this is intended as
a dirty hack to make some cases work while other things remain broken,
or whether you believe that OID_RANGE_EQ_OP. If it's the latter, this
needs a much better explanation. You probably realize that
"Attention: cannot merge using non-equality operator 3890" is not a
compelling justification, and that hard-coding all of these things
here doesn't look good.

In general, this patch needs both user-facing documentation and more
and better code comments. I would suggest writing the user-facing
documentation soon. It is pretty clear that you've got the basics of
this working, but it's impossible to understand what the semantics are
supposed to be except by staring at the code until you figure it out,
or running experiments. People who are interested in this
functionality are more likely to provide useful feedback if there's
something they can read and go "yeah, that looks right" or "wait, that
sounds wrong." Also, there may be places where, in the process of
documenting, you realize that things should be changed to make them
more consistent or easier to understand. Adding a developer README to
the patch might be good, too.

With respect to this specific point, I'm wondering if the patch is
trying to handle two cases in somewhat different ways -- one where
there are actual range types involved and the other where there are
two different columns that can be view as constituting a range. That
might explain the special-casing here, but if so there is probably a
need to clearly distinguish those cases much earlier, probably
someplace in the planner, not just at execution time.

+static Datum
+getLower(Datum range, TypeCacheEntry *typcache)
...
+static Datum
+getUpper(Datum range, TypeCacheEntry *typcache)

Anything that calls both of these functions is going to call
range_deserialize() twice, which seems inefficient.

+ if (node->sweepline < currP1)

This looks very strange. Those are just Datums. Typically what you'd
do is determine based on the datatype which opclass's comparator to
use and then call that. I'm not sure what purpose could be served by
comparing Datums directly. Maybe it would give you sensible answers
if these are integers, but not if they're pointers and probably not
even if they are native floats.

+ // XXX PEMOSER Manually fix sort operation of second attribute
(former time, now upper(time))
+ // We must fix that in general... this is just a proof of concept
brute-force solution!
+ if (plannode->plan.lefttree->type == T_ProjectSet) {
+ plannode->sortOperators[0] = 97; // 97 means "<" for int4, it was
"<" for int4range
+ }

Aside from the fact that this shouldn't be hard-coded, this shouldn't
be in the executor at all. The planner should be figuring this out
for us. If the planner is making a bad decision, then it needs to be
fixed so that it makes a good decision. Just trying to work around
the wrong decision elsewhere in the code won't lead to anything good.

+ /*
+ * TEMPORAL NORMALIZE: To improve this, we would need to remove only
+ * temporal range types from the path key list, not all
+ */

If you can do a temporal join that has leading columns being joined in
a standard way, then it might be possible to truncate the PathKey list
to include just the leading columns, provided the join preserves
ordering on those columns -- but once you get to the first column in
the PathKey where ordering is not preserved, any columns after that
point are certainly no longer part of the PathKey.

+ /*
+ * XXX PEMOSER NORMALIZE needs a result node above to properly
+ * handle target lists, functions and constants
+ * Maybe we need to refine this like in create_unique_plan:
+ * "If the top plan node can't do projections..."
+ */
+ if (best_path->jpath.jointype == JOIN_TEMPORAL_NORMALIZE)
+ join_plan = make_result(tlist, NULL, join_plan);

Actually, I think what you need to do is make sure that MergeJoin can
still project in all cases after your changes. Breaking that only for
the temporal normalize case isn't going to be acceptable, and it
shouldn't be hard to avoid having such a restriction. It's "just" a
matter of figuring out how a bunch of fiddly executor bits work...

+ // XXX PEMOSER Hardcoded NORMALIZE detection... change this. Read
the note below...

What note below?

+ /* Temporal NORMALIZE appends an expression to compare temporal bounds */
+ if (normalizeVars)
+ {
+ A_Expr *e;
+ e = makeSimpleA_Expr(AEXPR_OP, "=",
+ (Node *) copyObject(linitial(normalizeVars)),
+ (Node *) copyObject(lsecond(normalizeVars)),
+ -1);
+ andargs = lappend(andargs, e);
+ }

You absolutely cannot do stuff like this in parse analysis. It's
doubtful whether it's acceptable in general to include hard-coded
knowledge of = as a general point, but it's certainly not OK to do it
in parse analysis. Parse analysis's job is to determine the objects
to which the query refers, NOT to massage it as a preparation for
further optimization.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company