Horton

Thread

Commits

  1. Add a debugging option to stress-test outfuncs.c and readfuncs.c.

  2. Fix some minor issues exposed by outfuncs/readfuncs testing.

  3. Fix some probably-minor oversights in readfuncs.c.

  1. More deficiencies in outfuncs/readfuncs processing

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-16T23:03:12Z

    The report in

https://www.postgresql.org/message-id/flat/3690074f-abd2-56a9-144a-aa5545d7a291%40postgrespro.ru

set off substantial alarm bells for me about whether outfuncs/readfuncs
processing had any additional problems we'd failed to notice.  I thought
that to investigate that, it'd be a good idea to invent a debugging option
comparable to COPY_PARSE_PLAN_TREES, except it passes all parse and plan
trees through nodeToString + stringToNode rather than copyObject.  I did
so, and darn if a number of creepy-crawlies didn't get shaken out of the
woodwork immediately.  The first attached patch adds that debugging option
(plus some hackery I'll explain in a moment).  I propose that some form of
this should go into HEAD and we should configure at least one buildfarm
animal to enable it.  The second attached patch fixes the bugs I found;
parts of it need to get back-patched.

This debugging option also exposes the XMLNAMESPACES issue shown in the
aforementioned thread.  So the patch shown there needs to go in first,
or you get a core dump in xml-enabled builds.  But the sum total of all
three patches does pass make check-world.

The hackery in patch 0001 has to do with copying queryId, stmt_location,
and stmt_len fields forward across nodeToString + stringToNode.  The
core regression tests pass fine without that, but pg_stat_statements
falls over, because it needs that data to survive parse analysis as
well as planning.

As far as queryId goes, I'm satisfied with the hack shown here of just
having pg_rewrite_query propagate the field forward across the test.
There's a case to be made for fixing it by storing queryId in stored rules
instead, but that would require a far higher commitment to stability and
universality of the queryId computation than we've made so far.  Such a
change seems like something to consider separately if at all.  (Note that
no hack is needed for plan trees, because _outPlannedStmt/_readPlannedStmt
already transmit the queryId for a plan.)

The location fields are a much more ticklish matter, because the behavior
for those ties into a bunch of historical and possible future behaviors.
Currently, although outfuncs.c prints node location fields just fine,
readfuncs.c ignores the stored values and inserts -1.  The reason for
that is to be able to distinguish nodes that actually came from the
current query (for which the locations actually mean something with
respect to a query text we have at hand) from nodes that came from some
stored view or rule (for which we lack corresponding query text).  So
readfuncs.c marks nodes of the latter type as "unknown location" and all
is well.  As this patch stands, when the debug option is on, all nodes
coming out of the parser will have unknown locations, except for the
top-level Query or PlannedStmt node that we specially hacked.  That's not
great; a debugging option shouldn't risk behavioral changes like that.
Right now, we pass regression tests anyway because pretty much nothing
after parse analysis looks at the location fields, but there have been
discussions about changing that so as to get better execution-time
error reporting.

I thought for a bit about replacing those hacks with something like
this in readfuncs.c:

#ifdef WRITE_READ_PARSE_PLAN_TREES
#define READ_LOCATION_FIELD(fldname) READ_INT_FIELD(fldname)
#else
// existing definition of READ_LOCATION_FIELD
#endif

but that breaks the property of clearing the location info coming
in from stored rules, so it's unlikely to be satisfactory.  Another
idea is to have a runtime switch in readfuncs.c, allowing stringToNode
to treat location fields as it does now while there'd be an additional
entry point that would read location fields like plain ints.  Then
we'd use the latter for the test code.  This is a bit ugly but it might
be the best solution.

Patch 0002 addresses several more-or-less-independent issues that are
exposed by running the regression tests with patch 0001 activated:

* Query.withCheckOptions fails to propagate through write+read, because
it's intentionally ignored by outfuncs/readfuncs on the grounds that
it'd always be NIL anyway in stored rules.  This seems like premature
optimization of the worst sort, since it's not even saving very much:
if the assumption holds, what's suppressed from a stored Query is only
" :withCheckOptions <>", hardly a big savings.  And of course if the
assumption ever fails to hold, it's just broken, and broken in a non
obvious way too (you'd only notice if you expected a check option
failure and didn't get one).  So this is pretty dumb and I think we
ought to fix it by treating that field normally in outfuncs/readfuncs.
That'd require a catversion bump, but we only need to do it in HEAD.
The only plausible alternative is to change _equalQuery to ignore
withCheckOptions, which does not sound like a good idea at all.

* The system expects TABLEFUNC RTEs to have coltypes, coltypmods, and
colcollations lists, but outfuncs doesn't dump them and readfuncs
doesn't restore them.  This doesn't cause obvious failures, because
the only things that look at those fields are expandRTE() and
get_rte_attribute_type(), which are mostly used during parse analysis.
But expandRTE() is used in the rewriter and planner, so probably it's
possible to cause a crash or misbehavior with a stored view that returns
the result of an XMLTABLE FROM-item.  I've not tried to build a test case,
but I'm pretty sure we need to back-patch a fix for this as far back as
we have XMLTABLE.  Fortunately, because those fields are redundant with
the TableFunc node, we can just link to the lists we read in the TableFunc
node rather than force a catversion bump to store them separately.  (This
is pretty grotty, but since it's replicating the physical duplication
established by addRangeTableEntryForTableFunc, I think it's OK.)

* readfuncs.c omits read support for NamedTuplestoreScan plan nodes.
This accidentally fails to break parallel query because a query using
a named tuplestore would never be considered parallel-safe anyway,
so we don't have to ship it to workers.  Still, I'm pretty certain
this is somebody's sloppy oversight not an intentional omission,
and propose that we should back-patch that addition as far back as
NamedTuplestoreScan exists (ie v10).  There seem to be no other cases
where we've omitted plan node read support, so this one has little
excuse to live.

* Several places that convert a view RTE into a subquery RTE, or similar
manipulations, failed to clear out fields that were specific to the
original RTE type and should be zero in a subquery RTE.  This has no real
impact on the system, but since readfuncs.c will leave such fields as
zero, equalfuncs.c thinks the nodes are different leading to a reported
failure.  I'm satisfied with patching this in HEAD.  (You could
alternatively argue that we should change _equalRangeTblEntry to ignore
fields that are not supposed to be set based on the rtekind, but I don't
think that's a good idea.)

* BuildOnConflictExcludedTargetlist randomly set the resname of some
TargetEntries to "" not NULL.  outfuncs/readfuncs don't distinguish those
cases, and so the string will read back in as NULL ... but equalfuncs.c
does distinguish.  Perhaps we ought to try to make things more consistent
in this area --- but it's just useless extra code space for
BuildOnConflictExcludedTargetlist to not use NULL here, so I fixed it for
now by making it do so.  Again, changing this in HEAD seems sufficient.

Comments?

			regards, tom lane

  2. Re: More deficiencies in outfuncs/readfuncs processing

    Stephen Frost <sfrost@snowman.net> — 2018-09-17T10:42:56Z

    Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Patch 0002 addresses several more-or-less-independent issues that are
> exposed by running the regression tests with patch 0001 activated:
> 
> * Query.withCheckOptions fails to propagate through write+read, because
> it's intentionally ignored by outfuncs/readfuncs on the grounds that
> it'd always be NIL anyway in stored rules.  This seems like premature
> optimization of the worst sort, since it's not even saving very much:
> if the assumption holds, what's suppressed from a stored Query is only
> " :withCheckOptions <>", hardly a big savings.  And of course if the
> assumption ever fails to hold, it's just broken, and broken in a non
> obvious way too (you'd only notice if you expected a check option
> failure and didn't get one).  So this is pretty dumb and I think we
> ought to fix it by treating that field normally in outfuncs/readfuncs.
> That'd require a catversion bump, but we only need to do it in HEAD.
> The only plausible alternative is to change _equalQuery to ignore
> withCheckOptions, which does not sound like a good idea at all.

I'm fine with this change (as I believe I've said before...).  I agree
that it's just a minor optimization and shouldn't be an issue to remove
it.

Thanks!

Stephen