Previously the shutdown of a background worker that uses die() as
SIGTERM signal handler produced the log message "terminating
connection due to administrator command". This log message was
confusing because a background worker is not a connection.
This commit improves that log message to "terminating background
worker XXX due to administrator command" (XXX is replaced with
the name of the background worker). This is the same log message
as another SIGTERM signal handler bgworker_die() for a background
worker reports.
Author: Bharath Rupireddy
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/3f292fbb-f155-9a01-7cb2-7ccc9007ab3f@oss.nttdata.com
Through my misreading of what the existing code actually did,
commits 85c54287a et al. broke psql's behavior for the case where
"\c connstring" provides a password in the connstring. We should
use that password in such a case, but as of 85c54287a we ignored it
(and instead, prompted for a password).
Commit 94929f1cf fixed that in HEAD, but since I thought it was
cleaning up a longstanding misbehavior and not one I'd just created,
I didn't back-patch it.
Hence, back-patch the portions of 94929f1cf having to do with
password management. In addition to fixing the introduced bug,
this means that "\c -reuse-previous=on connstring" will allow
re-use of an existing connection's password if the connstring
doesn't change user/host/port. That didn't happen before, but
it seems like a bug fix, and anyway I'm loath to have significant
differences in this code across versions.
Also fix an error with the same root cause about whether or not to
override a connstring's setting of client_encoding. As of 85c54287a
we always did so; restore the previous behavior of overriding only
when stdin/stdout are a terminal and there's no environment setting
of PGCLIENTENCODING. (I find that definition a bit surprising, but
right now doesn't seem like the time to revisit it.)
Per bug #16746 from Krzysztof Gradek. As with the previous patch,
back-patch to all supported branches.
Discussion: https://postgr.es/m/16746-44b30e2edf4335d4@postgresql.org
The documentation omitted the critical tidbit that a keyword-array entry
is simply ignored if its corresponding value-array entry is NULL or an
empty string; it will *not* override any previously-obtained value for
the parameter. (See conninfo_array_parse().) I'd supposed that would
force the setting back to default, which is what led me into bug #16746;
but it doesn't.
While here, I couldn't resist the temptation to do some copy-editing,
both in the description of PQconnectdbParams() and in the section
about connection URI syntax.
Discussion: https://postgr.es/m/931505.1606618746@sss.pgh.pa.us
Buildfarm member topminnow failed when the test script attempted this
before the syslogger would have created the file. Back-patch to v12,
which introduced the test.
Commit 566372b3d fixed some race conditions involving concurrent
SimpleLruTruncate calls, but it introduced new ones in async.c.
A newly-listening backend could attempt to read Notify SLRU pages that
were in process of being truncated, possibly causing an error. Also,
the QUEUE_TAIL pointer could become set to a value that's not equal to
the queue position of any backend. While that's fairly harmless in
v13 and up (thanks to commit 51004c717), in older branches it resulted
in near-permanent disabling of the queue truncation logic, so that
continued use of NOTIFY led to queue-fill warnings and eventual
inability to send any more notifies. (A server restart is enough to
make that go away, but it's still pretty unpleasant.)
The core of the problem is confusion about whether QUEUE_TAIL
represents the "logical" tail of the queue (i.e., the oldest
still-interesting data) or the "physical" tail (the oldest data we've
not yet truncated away). To fix, split that into two variables.
QUEUE_TAIL regains its definition as the logical tail, and we
introduce a new variable to track the oldest un-truncated page.
Per report from Mikael Gustavsson. Like the previous patch,
back-patch to all supported branches.
Discussion: https://postgr.es/m/1b8561412e8a4f038d7a491c8b922788@smhi.se
Previously pg_stat_progress_cluster view reported the current block
number in heap scan as the number of heap blocks scanned (i.e.,
heap_blks_scanned). This reported number could be incorrect when
synchronize_seqscans is enabled, because it allowed the heap scan to
start at block in middle. This could result in wraparounds in the
heap_blks_scanned column when the heap scan wrapped around.
This commit fixes the bug by calculating the number of blocks from
the block that the heap scan starts at to the current block in scan,
and reporting that number in the heap_blks_scanned column.
Also, in pg_stat_progress_cluster view, previously heap_blks_scanned
could not reach heap_blks_total at the end of heap scan phase
if the last pages scanned were empty. This commit fixes the bug by
manually updating heap_blks_scanned to the same value as
heap_blks_total when the heap scan phase finishes.
Back-patch to v12 where pg_stat_progress_cluster view was introduced.
Reported-by: Matthias van de Meent
Author: Matthias van de Meent
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/CAEze2WjCBWSGkVfYag001Rc4+-nNLDpWM7QbyD6yPvuhKs-gYQ@mail.gmail.com
Previously test_shm_mq worker used the stripped-down version of die()
as the SIGTERM signal handler. This commit makes it use die(), instead,
to simplify the code.
In terms of the code, the difference between die() and the stripped-down
version previously used is whether the signal handler directly may call
ProcessInterrupts() or not. But this difference doesn't exist in
a background worker because, in bgworker, DoingCommandRead flag will
never be true and die() will never call ProcessInterrupts() directly.
Therefore test_shm_mq worker can safely use die(), like other bgworker
proceses (e.g., logical replication apply launcher or autoprewarm worker)
currently do.
Thanks to Craig Ringer for the report and investigation of the issue.
Author: Bharath Rupireddy
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/CAGRY4nxsAe_1k_9g5b47orA0S011iBoHsXHFMH7cg7HV0O1bwQ@mail.gmail.com
Previously worker_spi used its custom signal handlers for SIGHUP and
SIGTERM. This commit makes worker_spi use the standard signal handlers,
to simplify the code.
Note that die() is used as the standard SIGTERM signal handler in
worker_spi instead of SignalHandlerForShutdownRequest() or bgworker_die().
Previously the exit handling was only able to exit from within the main loop,
and not from within the backend code it calls. This is why die() needs to
be used here, so worker_spi can respond to SIGTERM signal while it's
executing a query.
Maybe we can say that it's a bug that worker_spi could not respond to
SIGTERM during query execution. But since worker_spi is a just example
of the background worker code, we don't do the back-patch.
Thanks to Craig Ringer for the report and investigation of the issue.
Author: Bharath Rupireddy
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/CALj2ACXDEZhAFOTDcqO9cFSRvrFLyYOnPKrcA1UG4uZn9hUAVg@mail.gmail.com
Discussion: https://postgr.es/m/CAGRY4nxsAe_1k_9g5b47orA0S011iBoHsXHFMH7cg7HV0O1bwQ@mail.gmail.com
Tablesync worker runs under a single transaction but in streaming mode, we
were committing the transaction on stream_stop, stream_abort, and
stream_commit. We need to avoid committing the transaction in a streaming
mode in tablesync worker.
In passing move the call to process_syncing_tables in
apply_handle_stream_commit after clean up of stream files. This will
allow clean up of files to happen before the exit of tablesync worker
which would otherwise be handled by one of the proc exit routines.
Author: Dilip Kumar
Reviewed-by: Amit Kapila and Peter Smith
Tested-by: Peter Smith
Discussion: https://postgr.es/m/CAHut+Pt4PyKQCwqzQ=EFF=bpKKJD7XKt_S23F6L20ayQNxg77A@mail.gmail.com
Reverts 27838981be (some comments are kept). Per discussion, it does
not seem safe to relax the lock level used for this; in order for it to
be safe, there would have to be memory barriers between the point we set
the flag and the point we set the trasaction Xid, which perhaps would
not be so bad; but there would also have to be barriers at the readers'
side, which from a performance perspective might be bad.
Now maybe this analysis is wrong and it *is* safe for some reason, but
proof of that is not trivial.
Discussion: https://postgr.es/m/20201118190928.vnztes7c2sldu43a@alap3.anarazel.de
If more distinct statements than pg_stat_statements.max are observed,
pg_stat_statements entries about the least-executed statements are
deallocated. This commit enables us to track the total number of times
those entries were deallocated. That number can be viewed in the
pg_stat_statements_info view that this commit adds. It's useful when
tuning pg_stat_statements.max parameter. If it's high, i.e., the entries
are deallocated very frequently, which might cause the performance
regression and we can increase pg_stat_statements.max to avoid those
frequent deallocations.
The pg_stat_statements_info view is intended to display the statistics
of pg_stat_statements module itself. Currently it has only one column
"dealloc" indicating the number of times entries were deallocated.
But an upcoming patch will add other columns (for example, the time
at which pg_stat_statements statistics were last reset) into the view.
Author: Katsuragi Yuta, Yuki Seino
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/0d9f1107772cf5c3f954e985464c7298@oss.nttdata.com
A prepared statement is re-analyzed and re-planned whenever database
objects used in the statement have undergone definitional changes or
the planner statistics of them have been updated. The former has been
documented from before, but the latter was not previously. This commit
adds the description about the latter case into the docs.
Author: Atsushi Torikoshi
Reviewed-by: Andy Fan, Fujii Masao
Discussion: https://postgr.es/m/3ac82f4817c9fe274a905c8a38d87bd9@oss.nttdata.com
In the various waiting phases of CREATE INDEX CONCURRENTLY (CIC) and
REINDEX CONCURRENTLY (RC), we wait for other processes to release their
snapshots; this is necessary in general for correctness. However,
processes doing CIC in other tables cannot possibly affect CIC or RC
done in "this" table, so we don't need to wait for those. This commit
adds a flag in MyProc->statusFlags to indicate that the current process
is doing CIC, so that other processes doing CIC or RC can ignore it when
waiting.
Note that this logic is only valid if the index does not access other
tables. For simplicity we avoid setting the flag if the index has a
column that's an expression, or has a WHERE predicate. (It is possible
to have expressional or partial indexes that do not access other tables,
but figuring that out would require more work.)
This flag can potentially also be used by processes doing REINDEX
CONCURRENTLY to be skipped; and by VACUUM to ignore processes in CIC or
RC for the purposes of computing an Xmin. That's left for future
commits.
Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Author: Dimitry Dolgov <9erthalion6@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/20200810233815.GA18970@alvherre.pgsql
Historically, psql has truncated the text of a column's default
expression at 128 characters. This is unlike any other behavior
in describe.c, and it's become particularly confusing now that
the limit is only applied to the expression proper and not to
the "generated always as (...) stored" text that may get wrapped
around it.
Excavation in our git history suggests that the original motivation
for this limit was not really to limit the display width (as I'd long
supposed), but to make it safe to use a fixed-width output buffer to
store the result. That implementation restriction is long gone of
course, but the limit remained. Let's just get rid of it.
While here, rearrange the logic about when to free the output string
so that it's not so dependent on unstated assumptions about the
possible values of attidentity and attgenerated.
Per bug #16743 from David Turon. Back-patch to v12 where GENERATED
came in. (Arguably we could take it back further, but I'm hesitant
to change the behavior of long-stable branches for this.)
Discussion: https://postgr.es/m/16743-7b1bacc4af76e7ad@postgresql.org
Break the per-index-type discussions into <sect2>'s so as to make
them more visually separate and easier to find. Improve the markup,
and make a couple of small wording adjustments.
This also fixes one stray reference to the now-deprecated point
operators <^ and >^.
Dagfinn Ilmari Mannsåker, reviewed by David Johnston and Jürgen Purtz
Discussion: https://postgr.es/m/877dukhvzg.fsf@wibble.ilmari.org
Up to now, we sent a ParameterStatus message to the client immediately
upon any change in the active value of any GUC_REPORT variable. This
was only barely okay when the feature was designed; now that we have
things like function SET clauses, there are very plausible use-cases
where a GUC_REPORT variable might change many times within a query
--- and even end up back at its original value, perhaps. Fortunately
most of our GUC_REPORT variables are unlikely to be changed often;
but there are proposals in play to enlarge that set, or even make it
user-configurable.
Hence, let's fix things to not generate more than one ParameterStatus
message per variable per query, and to not send any message at all
unless the end-of-query value is different from what we last reported.
Discussion: https://postgr.es/m/5708.1601145259@sss.pgh.pa.us
The function converted the first argument i.e. the number of tuples to
return into an unsigned integer which turns out to be huge number when
a negative value is passed. This causes the function to take much
longer time to execute. Instead, reject a negative value.
(If someone really wants to generate many more result rows, they
should consider adding a bigint or numeric variant.)
While at it, improve SQL test to test the number of tuples returned by
this function.
Author: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/CAG-ACPW3PUUmSnM6cLa9Rw4BEC5cEMKjX8Gogc8gvQcT3cYA1A@mail.gmail.com
The hint "Is another postmaster already running ..." should only be
printed for errors that are really about something else already using
the address. In other cases it is misleading. So only show that hint
if errno == EADDRINUSE.
Also, since Unix-domain sockets in the file-system namespace never
report EADDRINUSE for an existing file (they would just overwrite it),
the part of the hint saying "If not, remove socket file \"%s\" and
retry." can never happen, so remove it. Unix-domain sockets in the
abstract namespace can report EADDRINUSE, but in that case there is no
file to remove, so the hint doesn't work there either.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/6dee8574-b0ad-fc49-9c8c-2edc796f0033@2ndquadrant.com
This is a variant of the normal Unix-domain sockets that don't use the
file system but a separate "abstract" namespace. At the user
interface, such sockets are represented by names starting with "@".
Supported on Linux and Windows right now.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/6dee8574-b0ad-fc49-9c8c-2edc796f0033@2ndquadrant.com
The buildfarm animal walleye, running MinGW 8.1 has been having problems
ever since 697e1d02f and 913ec71d6 went in. This appears to be a bug in
assembler which was fixed in a later version.
For now, in order to get that animal running green again, let's just
define pg_attribute_cold and pg_attribute_hot to be empty macros on that
compiler. Hopefully, we can get the support of the owner of the animal to
upgrade to a less buggy compiler and revert this at a later date.
Discussion: https://postgr.es/m/286560.1606233316@sss.pgh.pa.us
currtid() and currtid2() are an undocumented set of functions whose sole
known user is the Postgres ODBC driver, able to retrieve the latest TID
version for a tuple given by the caller of those functions.
As used by Postgres ODBC, currtid() is a shortcut able to retrieve the
last TID loaded into a backend by passing an OID of 0 (magic value)
after a tuple insertion. This is removed in this commit, as it became
obsolete after the driver began using "RETURNING ctid" with inserts, a
clause supported since Postgres 8.2 (using RETURNING is better for
performance anyway as it reduces the number of round-trips to the
backend).
currtid2() is still used by the driver, so this remains around for now.
Note that this function is kept in its original shape for backward
compatibility reasons.
Per discussion with many people, including Andres Freund, Peter
Eisentraut, Álvaro Herrera, Hiroshi Inoue, Tom Lane and myself.
Bump catalog version.
Discussion: https://postgr.es/m/20200603021448.GB89559@paquier.xyz
Previously this code assumed that all IndexScan nodes supported
mark/restore, which is not true since it depends on optional index AM
support functions. This could lead to errors about missing support
functions in rare edge cases of mergejoins with no sort keys, where an
unordered non-btree index scan was placed on the inner path without a
protecting Materialize node. (Normally, the fact that merge join
requires ordered input would avoid this error.)
Backpatch all the way since this bug is ancient.
Per report from Eugen Konkov on irc.
Discussion: https://postgr.es/m/87o8jn50be.fsf@news-spur.riddles.org.uk
1fa22a43a was a quick fix for portability problem I introduced in
697e1d02f. 1fa22a43a adds a few more cases to the preprocessor logic than
I'd have liked. Andres Freund and Dagfinn Ilmari Mannsåker suggested a
better way to do this.
In passing, also adjust the only current usage of these macros so that the
macro comes before the function's return type in the declaration of the
function. This now matches what the definition of the function does.
Discussion: https://postgr.es/m/20200625163553.lt6wocbjhklp5pl4@alap3.anarazel.de
Discussion: https://postgr.es/m/87pn43bmok.fsf@wibble.ilmari.org
I'm having a hard time telling whether the letter of the C standard
requires this, but we do have a couple of buildfarm members that
throw warnings when this is not done. Oversight in c532d15dd.
Commit 0a2bc5d61e changed the order that permissions on the final and
transition functions of an aggregate are checked in. That shows up as a
difference in the order the LOG messages in this sepgsql regression test
are printed. Adjust the expected output.
Per buildfarm failure in rhinoceros.
Previously some index items have " ," (i.e., space + comma) in the docs
as follows. Since the space character before the comma is unnecessary,
this commit gets rid of that for the sake of consistency with other
index items.
parallel_leader_participation configuration parameter , Other Planner Options
Author: Fujii Masao
Reviewed-by: Euler Taveira
Discussion: https://postgr.es/m/e87b4ddf-1498-2850-bf55-519df3928fd4@oss.nttdata.com
While ereport() and elog() themselves are quite cheap when the
error message level is too low to be printed, some places need to do
substantial work before they can call those macros at all. To allow
optimizing away such setup work when nothing is to be printed, make
elog.c export a new function message_level_is_interesting(elevel)
that reports whether ereport/elog will do anything. Make use of that
in various places that had ad-hoc direct tests of log_min_messages etc.
Also teach ProcSleep to use it to avoid some work. (There may well
be other places that could usefully use this; I didn't search hard.)
Within elog.c, refactor a little bit to avoid having duplicate copies
of the policy-setting logic. When that code was written, we weren't
relying on the availability of inline functions; so it had some
duplications in the name of efficiency, which I got rid of.
Alvaro Herrera and Tom Lane
Discussion: https://postgr.es/m/129515.1606166429@sss.pgh.pa.us
This should fix the breakages caused by 697e1d02f, which seems to break
the build for GCC version < 5.
It seems, according to the GCC manual that __has_attribute is a "special
operator" and must be tested without any other conditions in the
preprocessor test.
Per recommendation from the GCC manual via Greg Nancarrow
Reported-by: Greg Nancarrow
Discussion: https://postgr.es/m/CAJcOf-euSu8fhC10v476o9dqnjqKysVs1_vRms-_fvajpZ3kFw@mail.gmail.com
Here we use a bit of preprocessor trickery to coax supporting compilers
into laying out their generated code so that the code that's in the same
branch as elog(ERROR)/ereport(ERROR) calls is moved away from the hot
path. Effectively, this reduces the size of the hot code meaning that it
can sit on fewer cache lines.
Performance improvements of between 10-15% have been seen on highly CPU
bound workloads using pgbench's TPC-b benchmark.
What's achieved here is very similar to putting the error condition inside
an unlikely() macro. For example;
if (unlikely(x < 0))
elog(ERROR, "invalid x value");
now there's no need to make use of unlikely() here as the common macro
used by elog and ereport will now see that elevel is >= ERROR and make use
of a pg_attribute_cold marked version of errstart().
When elevel < ERROR or if it cannot be determined to be constant, the
original behavior is maintained.
Author: David Rowley
Reviewed-by: Andres Freund, Peter Eisentraut
Discussion: https://postgr.es/m/CAApHDvrVpasrEzLL2er7p9iwZFZ%3DJj6WisePcFeunwfrV0js_A%40mail.gmail.com
For compilers supporting __has_attribute and __has_attribute (hot/cold).
__has_attribute is supported on gcc >= 5, clang >= 2.9 and icc >= 17.
A followup commit will implement some usages of these macros.
Author: David Rowley
Reviewed-by: Andres Freund, Peter Eisentraut
Discussion: https://postgr.es/m/CAApHDvrVpasrEzLL2er7p9iwZFZ%3DJj6WisePcFeunwfrV0js_A%40mail.gmail.com
While cancelling an autovacuum worker, we hold ProcArrayLock while
formatting a debugging log string. We can make this shorter by saving
the data we need to produce the message and doing the formatting outside
the locked region.
This isn't terribly critical, as it only occurs pretty rarely: when a
backend runs deadlock detection and it happens to be blocked by a
autovacuum running autovacuum. Still, there's no need to cause a hiccup
in ProcArrayLock processing, which can be very high-traffic in some
cases.
While at it, rework code so that we only print the string when it is
really going to be used, as suggested by Michael Paquier.
Discussion: https://postgr.es/m/20201118214127.GA3179@alvherre.pgsql
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Historically these were called >^ and <^, but that is inconsistent
with the similar box, polygon, and circle operators, which are named
|>> and <<| respectively. Worse, the >^ and <^ names are used for
*not* strict above/below tests for the box type.
Hence, invent new operators following the more common naming. The
old operators remain available for now, and are still accepted by
the relevant index opclasses too. But there's a deprecation notice,
so maybe we can get rid of them someday.
Emre Hasegeli, reviewed by Pavel Borisov
Discussion: https://postgr.es/m/24348.1587444160@sss.pgh.pa.us
Clarify that you can "insert" into a generated column as long as what
you're inserting is a DEFAULT placeholder.
Also, use ERRCODE_GENERATED_ALWAYS in place of ERRCODE_SYNTAX_ERROR;
there doesn't seem to be any reason to use the less specific errcode.
Discussion: https://postgr.es/m/9q0sgcr416t.fsf@gmx.us
A few sanity checks in funcapi.c were not mentioning all the possible
clauses for failure, confusing developers who fat-fingered catalog data
additions. Make the errors more detailed to avoid wasting time in
pinpointing mistakes.
Per complaint from Craig Ringer.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAMsr+YH7Kd87A3cU5m_wKo46HPQ46zFv5wesFNL0YWxkGhGv3g@mail.gmail.com
Copy.c has grown really large. Split it into more manageable parts:
- copy.c now contains only a few functions that are common to COPY FROM
and COPY TO.
- copyto.c contains code for COPY TO.
- copyfrom.c contains code for initializing COPY FROM, and inserting the
tuples to the correct table.
- copyfromparse.c contains code for reading from the client/file/program,
and parsing the input text/CSV/binary format into tuples.
All of these parts are fairly complicated, and fairly independent of each
other. There is a patch being discussed to implement parallel COPY FROM,
which will add a lot of new code to the COPY FROM path, and another patch
which would allow INSERTs to use the same multi-insert machinery as COPY
FROM, both of which will require refactoring that code. With those two
patches, there's going to be a lot of code churn in copy.c anyway, so now
seems like a good time to do this refactoring.
The CopyStateData struct is also split. All the formatting options, like
FORMAT, QUOTE, ESCAPE, are put in a new CopyFormatOption struct, which
is used by both COPY FROM and TO. Other state data are kept in separate
CopyFromStateData and CopyToStateData structs.
Reviewed-by: Soumyadeep Chakraborty, Erik Rijkers, Vignesh C, Andres Freund
Discussion: https://www.postgresql.org/message-id/8e15b560-f387-7acc-ac90-763986617bfb%40iki.fi
One can say "INSERT INTO tab(generated_col) VALUES (DEFAULT)" and not
draw an error. But the equivalent case for a multi-row VALUES list
always threw an error, even if one properly said DEFAULT in each row.
Fix that. While here, improve the test cases for nearby logic about
OVERRIDING SYSTEM/USER values.
Dean Rasheed
Discussion: https://postgr.es/m/9q0sgcr416t.fsf@gmx.us
Since we're assuming IEEE floats these days, there seems little
reason not to do this. It has the advantage that when the slope is
computed as infinite due to the presence of Inf coordinates, we get
saner behavior than before from line_construct(), and thence also
in some dependent operations such as finding the closest point.
Also fix line_construct() to special-case slope zero. The previous
coding got the right answer in most cases, but it could compute
C as NaN when the point has Inf coordinates.
Discussion: https://postgr.es/m/CAGf+fX70rWFOk5cd00uMfa__0yP+vtQg5ck7c2Onb-Yczp0URA@mail.gmail.com
"FPeq(infinity, infinity)" returned false, on account of getting NaN
when it subtracts the two inputs. Fix that by adding a separate
check for exact equality.
FPle() and FPge() similarly got the wrong answer for two like-signed
infinities. In those cases, we can just rearrange the comparisons
to avoid potentially subtracting infinities.
While the sibling functions FPne() etc accidentally gave the right
answers even with the internal NaN results, it seems best to make
similar adjustments to them to avoid depending on this.
FPeq() has to be converted to an inline function to avoid double
evaluations of its arguments, and I did the same for the others
just for consistency.
In passing, make the handling of NaN cases in line_eq() and
point_eq_point() simpler and easier to reason about, and perhaps
faster.
This results in just one visible regression test change: slope()
now gives DBL_MAX for two inputs of (inf,1e300), which is consistent
with what it does for (1e300,inf), so that seems like a bug fix.
Discussion: https://postgr.es/m/CAGf+fX70rWFOk5cd00uMfa__0yP+vtQg5ck7c2Onb-Yczp0URA@mail.gmail.com
Add another edge-case value to "point_tbl", and add a test for
the line(point, point) function.
Some of the behaviors exposed here are wrong, but the idea of
committing this separately is to memorialize what we were getting,
and to allow easier inspection of the behavior changes caused by
upcoming patches.
Kyotaro Horiguchi (line() test added by me)
Discussion: https://postgr.es/m/CAGf+fX70rWFOk5cd00uMfa__0yP+vtQg5ck7c2Onb-Yczp0URA@mail.gmail.com
Heikki Linnakangas