The node types A_Const, Constraint, and A_Expr had custom output
functions, but no read functions were implemented so far.
The A_Expr output format had to be tweaked a bit to make it easier to
parse.
Be a bit more cautious about applying strncmp to unterminated strings.
Also error out if an unrecognized enum value is found in each case,
instead of just printing a placeholder value. That was maybe ok for
debugging but won't work if we want to have robust round-tripping.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
The node tokenizer went out of its way to store BitString node values
without the leading 'b'. But everything else in the system stores the
leading 'b'. This would break if a BitString node is
read-printed-read.
Also, the node tokenizer didn't know that BitString node tokens could
also start with 'x'.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
The main parser checks whether a literal fits into an int when
deciding whether it should be put into an Integer or Float node. The
parser processes integer literals without signs. So a most-negative
integer literal will not fit into Integer and will end up as a Float
node.
The node tokenizer did this differently. It included the sign when
checking whether the literal fit into int. So a most-negative integer
would indeed fit that way and end up as an Integer node.
In order to preserve the node structure correctly, we need the node
tokenizer to also analyze integer literals without sign.
There are a number of test cases in the regression tests that have a
most-negative integer argument of some utility statement, so this
issue is easily reproduced under WRITE_READ_PARSE_PLAN_TREES.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
Because index creation does not go through heap_create_with_catalog() we
didn't call pgstat_create_relation(), leading to index stats of a newly
created realtion not getting dropped during rollback. To fix, move the
pgstat_create_relation() to heap_create(), which indexes do use.
Similarly, because dropping an index does not go through
heap_drop_with_catalog(), we didn't drop index stats when the transaction
dropping an index committed. Here there's no convenient common path for
indexes and relations, so index_drop() now calls pgstat_drop_relation().
Add tests for transactional index stats handling.
Author: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/51bbf286-2b4a-8998-bd12-eaae4b765d99@amazon.com
Backpatch: 15-, like 8b1dccd37c, which introduced the bug
We previously thought that allowing such cases can confuse users when they
specify DROP TABLES IN SCHEMA but that doesn't seem to be the case based
on discussion. This helps to uplift the restriction during
ALTER TABLE ... SET SCHEMA which used to ensure that we couldn't end up
with a publication having both a schema and the same schema's table.
To allow this, we need to forbid having any schema on a publication if
column lists on a table are specified (and vice versa). This is because
otherwise we still need a restriction during ALTER TABLE ... SET SCHEMA to
forbid cases where it could lead to a publication having both a schema and
the same schema's table with column list.
Based on suggestions by Peter Eisentraut.
Author: Hou Zhijie and Vignesh C
Reviewed-By: Peter Smith, Amit Kapila
Backpatch-through: 15, where it was introduced
Discussion: https://postgr.es/m/2729c9e2-9aac-8cda-f2f4-34f2bcc18f4e@enterprisedb.com
This may be a bit too subtle, but removing that word from there makes
this clause no longer a perfect parallel of the GRANT variant "ALL
TABLES IN SCHEMA": indeed, for publications what we record is the schema
itself, not the tables therein, which means that any tables added to the
schema in the future are also published. This is completely different
to what GRANT does, which is affect only the tables that exist when the
command is executed.
There isn't resounding support for this change, but there are a few
positive votes and no opposition. Because the time to 15 RC1 is very
short, let's get this out now.
Backpatch to 15.
Discussion: https://postgr.es/m/2729c9e2-9aac-8cda-f2f4-34f2bcc18f4e
Autoconf is showing its age, fewer and fewer contributors know how to wrangle
it. Recursive make has a lot of hard to resolve dependency issues and slow
incremental rebuilds. Our home-grown MSVC build system is hard to maintain for
developers not using Windows and runs tests serially. While these and other
issues could individually be addressed with incremental improvements, together
they seem best addressed by moving to a more modern build system.
After evaluating different build system choices, we chose to use meson, to a
good degree based on the adoption by other open source projects.
We decided that it's more realistic to commit a relatively early version of
the new build system and mature it in tree.
This commit adds an initial version of a meson based build system. It supports
building postgres on at least AIX, FreeBSD, Linux, macOS, NetBSD, OpenBSD,
Solaris and Windows (however only gcc is supported on aix, solaris). For
Windows/MSVC postgres can now be built with ninja (faster, particularly for
incremental builds) and msbuild (supporting the visual studio GUI, but
building slower).
Several aspects (e.g. Windows rc file generation, PGXS compatibility, LLVM
bitcode generation, documentation adjustments) are done in subsequent commits
requiring further review. Other aspects (e.g. not installing test-only
extensions) are not yet addressed.
When building on Windows with msbuild, builds are slower when using a visual
studio version older than 2019, because those versions do not support
MultiToolTask, required by meson for intra-target parallelism.
The plan is to remove the MSVC specific build system in src/tools/msvc soon
after reaching feature parity. However, we're not planning to remove the
autoconf/make build system in the near future. Likely we're going to keep at
least the parts required for PGXS to keep working around until all supported
versions build with meson.
Some initial help for postgres developers is at
https://wiki.postgresql.org/wiki/Meson
With contributions from Thomas Munro, John Naylor, Stone Tickle and others.
Author: Andres Freund <andres@anarazel.de>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Author: Peter Eisentraut <peter@eisentraut.org>
Reviewed-By: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/20211012083721.hvixq4pnh2pixr3j@alap3.anarazel.de
If the ps display is not cleared at this point, the process could
continue displaying "recovering NNN" even if handling end-of-recovery
steps. df9274a has tackled that by providing some information with the
end-of-recovery checkpoint but 7ff23c6 has nullified the effect of the
first commit.
Per a suggestion from Justin, just clear the ps display when we are done
with recovery, so as no incorrect information is displayed. This may
get extended in the future, but for now restore the pre-7ff23c6
behavior.
Author: Justin Prysby
Discussion: https://postgr.es/m/20220913223954.GU31833@telsasoft.com
Backpatch-through: 15
This commit updates two code paths to use pg_lfind32() introduced by
b6ef167 with TransactionId arrays:
- At the end of TransactionIdIsInProgress(), when checking for the case
of still running but overflowed subxids.
- XidIsConcurrent(), when checking for a serializable conflict.
These cases are less impactful than 37a6e5d, but a bit of
micro-benchmarking of this API shows that linear search speeds up by
~20% depending on the number of items involved (x86_64 and amd64 looked
at here).
Author: Nathan Bossart
Reviewed-by: Richard Guo, Michael Paquier
Discussion: https://postgr.es/m/20220901185153.GA783106@nathanxps13
We check that the ICU locale is only specified if the ICU locale
provider is selected. But we did that too early. We need to wait
until we load the settings of the template database, since that could
also set what the locale provider is.
Reported-by: Marina Polyakova <m.polyakova@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/9ba4cd1ea6ed6b7b15c0ff15e6f540cd@postgrespro.ru
For publication schemas (OBJECT_PUBLICATION_NAMESPACE) and user
mappings (OBJECT_USER_MAPPING), pg_get_object_address() checked the
array length of the second argument, but not of the first argument.
If the first argument was too long, it would just silently ignore
everything but the first argument. Fix that by checking the length of
the first argument as well.
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/caaef70b-a874-1088-92ef-5ac38269c33b%40enterprisedb.com
Previously the following snprintf() wrappers:
* ReplicationSlotNameForTablesync()
* ReplicationOriginNameForTablesync()
... used int as a second argument of snprintf() while the actual type of it
is size_t. Although it doesn't fail at present better replace it with Size
for consistency with the rest of the system.
Author: Aleksander Alekseev
Reviewed-By: Peter Smith
Discussion: https://postgr.es/m/CAHut%2BPsa8hhfSE6ozUK-ih7GkQziAVAf4f3bqiXEj2nQiu-43g%40mail.gmail.com
expression_tree_walker and allied functions have traditionally
declared their callback functions as, say, "bool (*walker) ()"
to allow for variation in the declared types of the callback
functions' context argument. This is apparently going to be
forbidden by the next version of the C standard, and the latest
version of clang warns about that. In any case it's always
been pretty poor for error-detection purposes, so fixing it is
a good thing to do.
What we want to do is change the callback argument declarations to
be like "bool (*walker) (Node *node, void *context)", which is
correct so far as expression_tree_walker and friends are concerned,
but not change the actual callback functions. Strict compliance with
the C standard would require changing them to declare their arguments
as "void *context" and then cast to the appropriate context struct
type internally. That'd be very invasive and it would also introduce
a bunch of opportunities for future bugs, since we'd no longer have
any check that the correct sort of context object is passed by outside
callers or internal recursion cases. Therefore, we're just going
to ignore the standard's position that "void *" isn't necessarily
compatible with struct pointers. No machine built in the last forty
or so years actually behaves that way, so it's not worth introducing
bug hazards for compatibility with long-dead hardware.
Therefore, to silence these compiler warnings, introduce a layer of
macro wrappers that cast the supplied function name to the official
argument type. Thanks to our use of -Wcast-function-type, this will
still produce a warning if the supplied function is seriously
incompatible with the required signature, without going as far as
the official spec restriction does.
This method fixes the problem without any need for source code changes
outside nodeFuncs.h/.c. However, it is an ABI break because the
physically called functions now have names ending in "_impl". Hence
we can only fix it this way in HEAD. In the back branches, we'll have
to settle for disabling -Wdeprecated-non-prototype.
Discussion: https://postgr.es/m/CA+hUKGKpHPDTv67Y+s6yiC8KH5OXeDg6a-twWo_xznKTcG0kSA@mail.gmail.com
Fix selfuncs.h cpluspluscheck complaint, without reintroducing a
parameter name inconsistency (restore the original declaration names,
and then make corresponding function definitions consistent with that).
Oversight in commit a601366a.
Author: Peter Geoghegan <pg@bowt.ie>
Reported-By: Andres Freund <andres@anarazel.de>
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in optimizer, parser,
utility, libpq, and "commands" code, as well as in remaining library
code. Do the same for all code related to frontend programs (with the
exception of pg_dump/pg_dumpall related code).
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy. Later commits will handle
ecpg and pg_dump/pg_dumpall.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
clang 15+ will issue a set-but-not-used warning when the only
use of a variable is in autoincrements (e.g., "foo++;").
That's perfectly sensible, but it detects a few more cases that
we'd not noticed before. Silence the warnings with our usual
methods, such as PG_USED_FOR_ASSERTS_ONLY, or in one case by
actually removing a useless variable.
One thing that we can't nicely get rid of is that with %pure-parser,
Bison emits "yynerrs" as a local variable that falls foul of this
warning. To silence those, I inserted "(void) yynerrs;" in the
top-level productions of affected grammars.
Per recently-established project policy, this is a candidate
for back-patching into out-of-support branches: it suppresses
annoying compiler warnings but changes no behavior. Hence,
back-patch to 9.5, which is as far as these patches go without
issues. (A preliminary check shows that the prior branches
need some other set-but-not-used cleanups too, so I'll leave
them for another day.)
Discussion: https://postgr.es/m/514615.1663615243@sss.pgh.pa.us
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in storage, catalog,
access method, executor, and logical replication code, as well as in
miscellaneous utility/library code.
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy. Later commits will do the
same for other parts of the codebase.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
Make sure that function declarations use names that exactly match the
corresponding names from function definitions. Having parameter names
that are reliably consistent in this way will make it easier to reason
about groups of related C functions from the same translation unit as a
module. It will also make certain refactoring tasks easier.
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy. Later commits will do the
same for other parts of the codebase.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
Adjust a handful of remaining function prototypes that were overlooked
by recent commit bc2187ed. This oversight wasn't caught by clang-tidy
because the functions in question are only built in custom REG_DEBUG
builds.
Author: Peter Geoghegan <pg@bowt.ie>
Reported-By: Tom Lane <tgl@sss.pgh.pa.us>
Make regex code consistently use named parameters in function
declarations. Also make sure that parameter names from each function's
declaration match corresponding definition parameter names.
This makes Henry Spencer's regex code follow Postgres coding standards.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
The header comment for get_cheapest_group_keys_order() claimed that the
output arguments were set to a newly allocated list which may be freed by
the calling function, however, this was not always true as the function
would simply leave these arguments untouched in some cases.
This tripped me up when working on 1349d2790 as I mistakenly assumed I
could perform a list_concat with the output parameters. That turned out
bad due to list_concat modifying the original input lists.
In passing, make it more clear that the number of distinct values is
important to reduce tiebreaks during sorts. Also, explain what the
n_preordered parameter means.
Backpatch-through: 15, where get_cheapest_group_keys_order was introduced.
If role A is a direct or indirect member of role B but does not inherit
B's privileges (because at least one relevant grant was created WITH
INHERIT FALSE) then A should not be permitted to bypass privilege
checks that require the privileges of B. For example, A can't change
the privileges of objects owned by B, nor can A drop those objects.
However, up until now, it's been possible for A to change default
privileges for role B. That doesn't seem to be correct, because a
non-inherited role grant is only supposed to permit you to assume
the identity of the granted role via SET ROLE, and should not
otherwise permit you to exercise the privileges of that role. Most
places followed that rule, but this case was an exception.
This could be construed as a security vulnerability, but it does not
seem entirely clear cut, since older branches were fuzzy about the
distinction between is_member_of_role() and has_privs_of_role() in
a number of other ways as well. Because of this, and because
user-visible behavior changes in minor releases are to be avoided
whenever possible, no back-patch.
Discussion: http://postgr.es/m/CA+TgmobG_YUP06R_PM_2Z7wR0qv_52gQPHD8CYXbJva0cf0E+A@mail.gmail.com
The API contract for planstate_tree_walker() callbacks is that they
take a PlanState pointer and a context pointer. Somebody figured
they could save a couple lines of code by ignoring that, and passing
ExecShutdownNode itself as the walker even though it has but one
argument. Somewhat remarkably, we've gotten away with that so far.
However, it seems clear that the upcoming C2x standard means to
forbid such cases, and compilers that actively break such code
likely won't be far behind. So spend the extra few lines of code
to do it honestly with a separate walker function.
In HEAD, we might as well go further and remove ExecShutdownNode's
useless return value. I left that as-is in back branches though,
to forestall complaints about ABI breakage.
Back-patch, with the thought that this might become of practical
importance before our stable branches are all out of service.
It doesn't seem to be fixing any live bug on any currently known
platform, however.
Discussion: https://postgr.es/m/208054.1663534665@sss.pgh.pa.us
Since c7aba7c, the transform method used during parse analysis of a
subcripting construct has moved from transformAssignmentSubscripts() to
the per-type transform method (arrays or arbitrary types) the step that
checks for slicing support, but transformAssignmentSubscripts() has kept
traces of it. Removing it simplifies the code, so let's clean up all
that.
Author: Zhang Mingli
Reviewed-by: Richard Guo
Discussion: https://postgr.es/m/0d7041ac-c704-48ad-86fd-e05feddf08ed@Spark
Make reorderbuffer.h function declarations consistently use named
parameters. Also make sure that the declarations use names that match
corresponding names from function definitions in reorderbuffer.c. This
makes the definitions easier to follow, especially in the case of
functions that happen to have adjoining arguments of the same type.
This patch was written with help from clang-tidy. Specifically, its
"readability-inconsistent-declaration-parameter-name" check and its
"readability-named-parameter" check were used.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/3955318.1663377656@sss.pgh.pa.us
So far they were created below CacheMemoryContext. However, that's not
guaranteed to exist in all situations, leading to memory contexts created as
top-level contexts. There isn't actually a good reason anymore to create them
below CacheMemoryContext, so just creating them below TopMemoryContext seems
the best approach.
Reported-by: Reid Thompson <reid.thompson@crunchydata.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Author: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Discussion: https://postgr.es/m/b948b729-42fe-f88c-2f4a-0e65d84c049b@amazon.com
Backpatch: 15-
Since Windows 10 1703, it is additionally necessary to pass a flag
called FILE_MAP_LARGE_PAGES to MapViewOfFile() to enable large pages at
map time. This flag is ignored on older versions of Windows, where
large pages should still be able to work properly without setting it.
Note that the flag would be set only for binaries that knew about it at
compile-time, which should be more or less all the Windows environments
these days.
Since 495ed0e, Windows 10 is the minimum version of Windows supported by
Postgres, making this change easy to reason about on HEAD. Per
discussion, no backpatch is done for the moment.
Reported-by: Okano Naoki
Author: Thomas Munro
Reviewed-by: Tom Lane, Michael Paquier, Julien Rouhaud
Discussion: https://postgr.es/m/17448-0a96583a67edb1f7@postgresql.org
Check in CREATE DATABASE and initdb that the selected encoding is
supported by ICU. Before, they would pass but users would later get
an error from the server when they tried to use the database.
Also document that initdb sets the encoding to UTF8 by default if the
ICU locale provider is chosen.
Author: Marina Polyakova <m.polyakova@postgrespro.ru>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://www.postgresql.org/message-id/6dd6db0984d86a51b7255ba79f111971@postgrespro.ru
After commit cc2c7d65fc added this flag,
failure to reset it caused assertion failures. In non-assert builds, it
made the system fail to achieve the objectives listed in that commit;
chiefly, we might emit a spurious log message. Back-patch to v15, where
that commit first appeared.
Bharath Rupireddy and Kyotaro Horiguchi. Reviewed by Dilip Kumar,
Nathan Bossart and Michael Paquier. Reported by Dilip Kumar.
Discussion: https://postgr.es/m/CAFiTN-sE3ry=ycMPVtC+Djw4Fd7gbUGVv_qqw6qfzp=JLvqT3g@mail.gmail.com
Commit ecaf7c5df5 removed gram.h from the backend's generated-headers
target. In LLVM builds, this leads to loss of dependency information
when generating .bc files. To fix, add a rule that mirrors ad-hoc .o
dependencies for .bc files as well.
Per cfbot (no buildfarm failures reported)
Analysis by Tom Lane and Andres Freund
Proposed fix by Andres Freund
Discussion: https://www.postgresql.org/message-id/20220914210427.y26tkagmxo5wwbvp%40awork3.anarazel.de
Referring to the WAL as just "log" invites confusion with the
postmaster log, so avoid doing that in docs and error messages.
Also shorten "WAL segment file" to just "WAL file" in various
places.
Bharath Rupireddy, reviewed by Nathan Bossart and Kyotaro Horiguchi
Discussion: https://postgr.es/m/CALj2ACUeXa8tDPaiTLexBDMZ7hgvaN+RTb957-cn5qwv9zf-MQ@mail.gmail.com
In 29f45e299, we added support for optimizing the execution of NOT
IN(values) by using a hash table instead of a linear search over the
array. That commit neglected to update the header comment for
convert_saop_to_hashed_saop() to mention this fact. Here we fix that.
Author: James Coleman
Discussion: https://postgr.es/m/CAAaqYe99NUpAPcxgchGstgM23fmiGjqQPot8627YgkBgNt=BfA@mail.gmail.com
Backpatch-through: 15, where 29f45e299 was added.
Various bits of code were declaring signal handlers manually,
using "int signum" or variants of that. We evidently have no
platforms where that's actually wrong, but let's use our
SIGNAL_ARGS macro everywhere anyway. If nothing else, it's
good for finding signal handlers easily.
No need for back-patch, since this is just cosmetic AFAICS.
Discussion: https://postgr.es/m/2684964.1663167995@sss.pgh.pa.us
This appears to be a merge mistake in 96ef3237bf. We could put it
back the way it was before JSON_TABLE and it'd be two lines shorter, but
it's likely that JSON_TABLE will be back and will prefer things this
way. It makes no other difference in practice.
Backpatch to 15.
Reported by Ranier Vilela
Discussion: https://postgr.es/m/CAEudQAr4nOcNQskC4oBEZN4S+4heJ=1ch_ZKOxU+_Ef-FQSf-g@mail.gmail.com
This header is semi-private, being used only in files related to
raw parsing, so move to the backend directory where those files
live. This allows removal of Makefile rules that symlink gram.h to
src/include/parser, since gramparse.h can now include gram.h from
within the same directory. This has the side-effect of no longer
installing gram.h and gramparse.h, but there doesn't seem to be a
good reason to continue doing so.
Per suggestion from Andres Freund and Peter Eisentraut
Discussion: https://www.postgresql.org/message-id/20220904181759.px6uosll6zbxcum5%40awork3.anarazel.de
PG_COMPRESSION_OPTION_LEVEL is removed from the compression
specification logic, and instead the compression level is always
assigned with each library's default if nothing is directly given. This
centralizes the checks on the compression methods supported by a given
build, and always assigns a default compression level when parsing a
compression specification. This results in complaining at an earlier
stage than previously if a build supports a compression method or not,
aka when parsing a specification in the backend or the frontend, and not
when processing it. zstd, lz4 and zlib are able to handle in their
respective routines setting up the compression level the case of a
default value, hence the backend or frontend code (pg_receivewal or
pg_basebackup) has now no need to know what the default compression
level should be if nothing is specified: the logic is now done so as the
specification parsing assigns it. It can also be enforced by passing
down a "level" set to the default value, that the backend will accept
(the replication protocol is for example able to handle a command like
BASE_BACKUP (COMPRESSION_DETAIL 'gzip:level=-1')).
This code simplification fixes an issue with pg_basebackup --gzip
introduced by ffd5365, where the tarball of the streamed WAL segments
would be created as of pg_wal.tar.gz with uncompressed contents, while
the intention is to compress the segments with gzip at a default level.
The origin of the confusion comes from the handling of the default
compression level of gzip (-1 or Z_DEFAULT_COMPRESSION) and the value of
0 was getting assigned, which is what walmethods.c would consider
as equivalent to no compression when streaming WAL segments with its tar
methods. Assigning always the compression level removes the confusion
of some code paths considering a value of 0 set in a specification as
either no compression or a default compression level.
Note that 010_pg_basebackup.pl has to be adjusted to skip a few tests
where the shape of the compression detail string for client and
server-side compression was checked using gzip. This is a result of the
code simplification, as gzip specifications cannot be used if a build
does not support it.
Reported-by: Tom Lane
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/1400032.1662217889@sss.pgh.pa.us
Backpatch-through: 15
guc.c has grown to be one of our largest .c files, making it
a bottleneck for compilation. It's also acquired a bunch of
knowledge that'd be better kept elsewhere, because of our not
very good habit of putting variable-specific check hooks here.
Hence, split it up along these lines:
* guc.c itself retains just the core GUC housekeeping mechanisms.
* New file guc_funcs.c contains the SET/SHOW interfaces and some
SQL-accessible functions for GUC manipulation.
* New file guc_tables.c contains the data arrays that define the
built-in GUC variables, along with some already-exported constant
tables.
* GUC check/assign/show hook functions are moved to the variable's
home module, whenever that's clearly identifiable. A few hard-
to-classify hooks ended up in commands/variable.c, which was
already a home for miscellaneous GUC hook functions.
To avoid cluttering a lot more header files with #include "guc.h",
I also invented a new header file utils/guc_hooks.h and put all
the GUC hook functions' declarations there, regardless of their
originating module. That allowed removal of #include "guc.h"
from some existing headers. The fallout from that (hopefully
all caught here) demonstrates clearly why such inclusions are
best minimized: there are a lot of files that, for example,
were getting array.h at two or more levels of remove, despite
not having any connection at all to GUCs in themselves.
There is some very minor code beautification here, such as
renaming a couple of inconsistently-named hook functions
and improving some comments. But mostly this just moves
code from point A to point B and deals with the ensuing
needs for #include adjustments and exporting a few functions
that previously weren't exported.
Patch by me, per a suggestion from Andres Freund; thanks also
to Michael Paquier for the idea to invent guc_funcs.c.
Discussion: https://postgr.es/m/587607.1662836699@sss.pgh.pa.us
Commit 3a0e385048 introduced a new path for unauthenticated bytes from
the client certificate to be printed unescaped to the logs. There are a
handful of these already, but it doesn't make sense to keep making the
problem worse. \x-escape any unprintable bytes.
The test case introduces a revoked UTF-8 certificate. This requires the
addition of the `-utf8` flag to `openssl req`. Since the existing
certificates all use an ASCII subset, this won't modify the existing
certificates' subjects if/when they get regenerated; this was verified
experimentally with
$ make sslfiles-clean
$ make sslfiles
Unfortunately the test can't be run in the CI yet due to a test timing
issue; see 55828a6b60.
Author: Jacob Champion <jchampion@timescale.com>
Discussion: https://www.postgresql.org/message-id/CAAWbhmgsvHrH9wLU2kYc3pOi1KSenHSLAHBbCVmmddW6-mc_=w@mail.gmail.com
The two strings are already a single palloc'd chunk, not freed; there's
no reason to allocate separate copies that have the same lifetime.
This code is only called in short-lived memory contexts (except in some
cases in TopTransactionContext, which is still short-lived enough not to
really matter), and typically only for short arrays, so the memory or
computation saved is likely negligible. However, let's fix it to avoid
leaving a bad example of code to copy. This is the only place I could
find where we're doing this with makeDefElem().
Reported-by: Junwang Zhao <zhjwpku@gmail.com>
Discussion: https://postgr.es/m/20220909142050.3vv2hjekppk265dd@alvherre.pgsql
The primary fix here is to fix has_matching_range() so it does not
reference ranges->values[-1] when nranges == 0. Similar problems existed
in AssertCheckRanges() too. It does not look like any of these problems
could lead to a crash as the array in question is at the end of the Ranges
struct, and values[-1] is memory that belongs to other fields in the
struct. However, let's get rid of these rather unsafe coding practices.
In passing, I (David) adjusted some comments to try to make it more clear
what some of the fields are for in the Ranges struct. I had to study the
code to find out what nsorted was for as I couldn't tell from the
comments.
Author: Ranier Vilela
Discussion: https://postgr.es/m/CAEudQAqJQzPitufX-jR=YUbJafpCDAKUnwgdbX_MzSc93wuvdw@mail.gmail.com
Backpatch-through: 14, where multi-range brin was added.
Commit c4c340088 changed geometric operators to use float4 and float8
functions, and handle NaN's in a better way. The circle sameness test
had a typo in the code which resulted in all comparisons with the left
circle having a NaN radius considered same.
postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
?column?
----------
t
(1 row)
This fixes the sameness test to consider the radius of both the left
and right circle.
Backpatch to v12 where this was introduced.
Author: Ranier Vilela <ranier.vf@gmail.com>
Discussion: https://postgr.es/m/CAEudQAo8dK=yctg2ZzjJuzV4zgOPBxRU5+Kb+yatFiddtQk6Rw@mail.gmail.com
Backpatch-through: v12
In commit f6c5edb8ab, we started to drop the replication origin slots
before tablesync worker exits to avoid consuming more slots than required.
We were dropping the replication origin in the same transaction where we
were marking the tablesync state as SYNCDONE. Now, if there is any error
after we have dropped the origin but before we commit the containing
transaction, the in-memory state of replication progress won't be rolled
back. Due to this, after the restart, tablesync worker can start streaming
from the wrong location and can apply the already processed transaction.
To fix this, we need to opportunistically drop the origin after marking
the tablesync state as SYNCDONE. Even, if the tablesync worker fails to
remove the replication origin before exit, the apply worker ensures to
clean it up afterward.
Reported by Tom Lane as per buildfarm.
Diagnosed-by: Masahiko Sawada
Author: Hou Zhijie
Reviewed-By: Masahiko Sawada, Amit Kapila
Discussion: https://postgr.es/m/20220714115155.GA5439@depesz.com
Discussion: https://postgr.es/m/CAD21AoAw0Oofi4kiDpJBOwpYyBBBkJj=sLUOn4Gd2GjUAKG-fw@mail.gmail.com
This change impacts the backend-side code in charge of starting a LDAP
TLS session. It is a bit sad that it is not possible to unify the WIN32
and non-WIN32 code paths, but the different number of arguments for both
discard this possibility.
This is similar to 47bd0b3, where this replaces the last function
loading that seems worth it, any others being either environment or
version-dependent.
Reported-by: Thomas Munro
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/Yx0rxpNgDh8tN4XA@paquier.xyz
The LDAP wiki states that the search message should be freed regardless
of the return value of ldap_search_s(), but we failed to do so in one
backend code path when searching LDAP with a filter. This is not
critical in an authentication code path failing in the backend as this
causes such the process to exit promptly, but let's be clean and free
the search message appropriately, as documented by upstream.
All the other code paths failing a LDAP operation do that already, and
somebody looking at this code in the future may miss what LDAP expects
with the search message.
Author: Zhihong Yu
Discussion: https://postgr.es/m/CALNJ-vTf5Y+8RtzZ4GjOGE9qWVHZ8awfhnFYc_qGm8fMLUNRAg@mail.gmail.com
Because of inadequate filtering, the check triggers were confusing the
search for action triggers in GetForeignKeyActionTriggers and vice-versa
in GetForeignKeyCheckTriggers; this confusion results in seemingly
random assertion failures, and can have real impact in non-asserting
builds depending on catalog order. Change these functions so that they
correctly ignore triggers that are not relevant to each side.
To reduce the odds of further problems, do not break out of the
searching loop in assertion builds. This break is likely to hide bugs;
without it, we would have detected this bug immediately.
This problem was introduced by f4566345cf, so backpatch to 15 where
that commit first appeared.
Author: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/20220908172029.sejft2ppckbo6oh5@awork3.anarazel.de
Discussion: https://postgr.es/m/4104619.1662663056@sss.pgh.pa.us
Since the retirement of some older buildfarm members, the oldest Bison
that gets regular testing is 2.3. MacOS ships that version, and will
continue doing so for the forseeable future because of Apple's policy
regarding GPLv3. While Mac users could use a package manager to install
a newer version, there is no compelling reason to force them do so at
this time.
Reviewed by Andres Freund
Discussion: https://www.postgresql.org/message-id/1097762.1662145681@sss.pgh.pa.us
This commit changes the following code paths to do direct system calls
to some WIN32 functions rather than loading them from an external
library, shaving some code in the process:
- Creation of restricted tokens in pg_ctl.c, introduced by a25cd81.
- QuerySecurityContextToken() in auth.c for SSPI authentication in the
backend, introduced in d602592.
- CreateRestrictedToken() in src/common/. This change is similar to the
case of pg_ctl.c.
Most of these functions were loaded rather than directly called because,
as mentioned in the code comments, MinGW headers were not declaring
them. I have double-checked the recent MinGW code, and all the
functions changed here are declared in its headers, so this change
should be safe. Note that I do not have a MinGW environment at hand so
I have not tested it directly, but that MSVC was fine with the change.
The buildfarm will tell soon enough if this change is appropriate or not
for a much broader set of environments.
A few code paths still use GetProcAddress() to load some functions:
- LDAP authentication for ldap_start_tls_sA(), where I am not confident
that this change would work.
- win32env.c and win32ntdll.c where we have a per-MSVC version
dependency for the name of the library loaded.
- crashdump.c for MiniDumpWriteDump() and EnumDirTree(), where direct
calls were not able to work after testing.
Reported-by: Thomas Munro
Reviewed-by: Justin Prysby
Discussion: https://postgr.es/m/CA+hUKG+BMdcaCe=P-EjMoLTCr3zrrzqbcVE=8h5LyNsSVHKXZA@mail.gmail.com
On failure in restoring a block image, no details were provided, while
it is possible to see failure with an inconsistent record state, a
failure in processing decompression or a failure in decompression
because a build does not support this option.
RestoreBlockImage() is used in two code paths in the backend code,
during recovery and when checking a page consistency after applying
masking, and both places are changed to consume the error message
produced by the internal routine when it returns a false status. All
the error messages are reported under ERRCODE_INTERNAL_ERROR, that gets
used also when attempting to access a page compressed by a method
not supported by the build attempting the decompression. This is
something that can happen in core when doing physical replication with
primary and standby using inconsistent build options, for example.
This routine is available since 2c03216d and it has never provided any
context about the error happening when it failed. This change is
justified even more after 57aa5b2, that introduced compression of FPWs
in WAL.
Reported-by: Justin Prysby
Author: Michael Paquier
Discussion: https://postgr.es/m/20220905002320.GD31833@telsasoft.com
Backpatch-through: 15
Add a new line to log reports from autovacuum (as well as VACUUM VERBOSE
output) that shows information about freezing. Emphasis is placed on
the total number of heap pages that had one or more tuples frozen by
VACUUM. The total number of tuples frozen is also shown.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Jeff Janes <jeff.janes@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznTY6D0zyE8VLrC6Gd4kh_HGAXxnTPtcOQOOsxzLx9zog@mail.gmail.com
5265e91fd changed MemoryContextContains to update it so that it works
correctly with the new MemoryChunk code added in c6e0fe1f2. However,
5265e91fd was done with the assumption that MemoryContextContains would
only ever be given pointers to memory that had been returned by one of our
MemoryContext allocators. It seems that's not true and many of our 32-bit
buildfarm animals are clearly showing that.
There are some code paths that call MemoryContextContains with a pointer
pointing part way into an allocated chunk. The example of this found by
the 32-bit buildfarm animals is the int2int4_sum() function. This
function returns transdata->sum, which is not a pointer to memory that was
allocated directly. This return value is then subsequently passed to
MemoryContextContains which causes it to crash due to it thinking the
memory directly prior to that pointer is a MemoryChunk. What's actually
in that memory is the field in the struct that comes prior to the "sum"
field. This problem didn't occur in 64-bit world because BIGINT is a
byval type and the code which was calling MemoryContextContains with the
bad pointer only does so with non-byval types.
Here, instead of reverting 5265e91fd and making MemoryContextContains
completely broken again, let's just make it always return false for now.
Effectively prior to 5265e91fd it was doing that anyway, this at least
makes that more explicit. The only repercussions of this with the current
MemoryContextContains calls are that we perform a datumCopy() when we
might not need to. This should make the 32-bit buildfarm animals happy
again and give us more time to consider a long-term fix.
Discussion: https://postgr.es/m/20220907130552.sfjri7jublfxyyi4%40jrouhaud
During ALTER TABLE ATTACH PARTITION, if the name of a parent's foreign
key constraint is already used on the partition, the code tries to
choose another one before the FK attributes list has been populated,
so the resulting constraint name was "<relname>__fkey" instead of
"<relname>_<attrs>_fkey". Repair, and add a test case.
Backpatch to 12. In 11, the code to attach a partition was not smart
enough to cope with conflicting constraint names, so the problem doesn't
exist there.
Author: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Discussion: https://postgr.es/m/20220901184156.738ebee5@karst
We should process completed IOs *before* trying to start more, so that
it is always possible to decode one more record when the decoded record
queue is empty, even if maintenance_io_concurrency is set so low that a
single earlier WAL record might have saturated the IO queue.
That bug was hidden because the effect of maintenance_io_concurrency was
arbitrarily clamped to be at least 2. Fix the ordering, and also remove
that clamp. We need a special case for 0, which is now treated the same
as recovery_prefetch=off, but otherwise the number is used directly.
This allows for testing with 1, which would have made the problem
obvious in simple test scenarios.
Also add an explicit error message for missing contrecords. It was a
bit strange that we didn't report an error already, and became a latent
bug with prefetching, since the internal state that tracks aborted
contrecords would not survive retrying, as revealed by
026_overwrite_contrecord.pl with this adjustment. Reporting an error
prevents that.
Back-patch to 15.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/20220831140128.GS31833%40telsasoft.com
perltidying a "##no critic" line moves the marker to where it becomes
useless. Put the line back to how it was, and protect it from further
malfeasance.
Per buildfarm member crake.
Previously Catalog.pm eval'd each individual hash reference
so that comments and whitespace can be preserved when running
reformat-dat-files. This is unnecessary when building, and we can save
~15% off the run time of genbki.pl by simply slurping and eval'-ing
the whole file at once. This saves a bit of time, especially in highly
parallel builds, since most build targets depend on this script's outputs.
Report and review by Andres Freund
Discussion: https://www.postgresql.org/message-id/CAFBsxsGW%3DWRbnxXrc8UqqR479XuxtukSFWV-hnmtgsbuNAUO6w%40mail.gmail.com
This commit raises a warning message for a combination of options
('copy_data = true' and 'origin = none') during CREATE/ALTER subscription
operations if the publication tables were also replicated from other
publishers.
During replication, we can skip the data from other origins as we have that
information in WAL but that is not possible during initial sync so we raise
a warning if there is such a possibility.
Author: Vignesh C
Reviewed-By: Peter Smith, Amit Kapila, Jonathan Katz, Shi yu, Wang wei
Discussion: https://www.postgresql.org/message-id/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9=9orqubhjcQ@mail.gmail.com
c6e0fe1f2 recently changed the way we store headers for allocated chunks
of memory. Prior to that commit, we stored a pointer to the owning
MemoryContext directly prior to the pointer to the allocated memory.
That's no longer true and c6e0fe1f2 neglected to update
MemoryContextContains() so that it correctly obtains the owning context
with the new method.
A side effect of this change and c6e0fe1f2, in general, is that it's even
less safe than it was previously to pass MemoryContextContains() an
arbitrary pointer which was not allocated by one of our MemoryContexts.
Previously some comments in MemoryContextContains() seemed to indicate
that the worst that could happen by passing an arbitrary pointer would be
a false positive return value. It seems to me that this was a rather
wishful outlook as we subsequently proceeded to subtract sizeof(void *)
from the given pointer and then dereferenced that memory. So it seems
quite likely that we could have segfaulted instead of returning a false
positive. However, it's not impossible that the memory sizeof(void *)
bytes before the pointer could have been owned by the process, but it's
far less likely to work now as obtaining a pointer to the owning
MemoryContext is less direct than before c6e0fe1f2 and will access memory
that's possibly much further away to obtain the owning MemoryContext.
Because of this, I took the liberty of updating the comment to warn
against any future usages of the function and checked the existing core
usages to ensure that we only ever pass in a pointer to memory allocated
by a MemoryContext.
Extension authors updating their code for PG16 who are using
MemoryContextContains should check to ensure that only NULL pointers and
pointers to chunks allocated with a MemoryContext will ever be passed to
MemoryContextContains.
Reported-by: Andres Freund
Discussion: https://postgr.es/m/20220905230949.kb3x2fkpfwtngz43@awork3.anarazel.de
Traditionally, in MEMORY_CONTEXT_CHECKING builds, we only ever marked a
sentinel byte just beyond the requested size if there happened to be
enough space on the chunk to do so. For Slab and Generation context
types, we only rounded the size of the chunk up to the next maxalign
boundary, so it was often not that likely that those would ever have space
for the sentinel given that the majority of allocation requests are going
to be for sizes which are maxaligned. For AllocSet, it was a little
different as smaller allocations are rounded up to the next power-of-2
value rather than the next maxalign boundary, so we're a bit more likely
to have space for the sentinel byte, especially when we get away from tiny
sized allocations such as 8 or 16 bytes.
Here we make more of an effort to allow space so that there is enough room
for the sentinel byte in more cases. This makes it more likely that we'll
detect when buggy code accidentally writes beyond the end of any of its
memory allocations.
Each of the 3 MemoryContext types has been changed as follows:
The Slab allocator will now always set a sentinel byte. Both the current
usages of this MemoryContext type happen to use chunk sizes which were on
the maxalign boundary, so these never used sentinel bytes previously.
For the Generation allocator, we now always ensure there's enough space in
the allocation for a sentinel byte.
For AllocSet, this commit makes an adjustment for allocation sizes which
are greater than allocChunkLimit. We now ensure there is always space for
a sentinel byte. We don't alter the sentinel behavior for request sizes
<= allocChunkLimit. Making way for the sentinel byte for power-of-2
request sizes would require doubling up to the next power of 2. Some
analysis done on the request sizes made during installcheck shows that a
fairly large portion of allocation requests are for power-of-2 sizes. The
amount of additional memory for the sentinel there seems prohibitive, so
we do nothing for those here.
Author: David Rowley
Discussion: https://postgr.es/m/3478405.1661824539@sss.pgh.pa.us
The addition of published column names forgot to filter on attisdropped,
leading to cases where you could see "........pg.dropped.1........"
or the like as a reportedly-published column.
While we're here, rewrite the new subquery to get a more efficient plan
for it.
Hou Zhijie, per report from Jaime Casanova. Back-patch to v15 where
the bug was introduced. (Sadly, this means we need a post-beta4
catversion bump before beta4 has even hit the streets. I see no
good alternative though.)
Discussion: https://postgr.es/m/Yxa1SU4nH2HfN3/i@ahch-to
Here we remove some dead code from CreateTriggerFiringOn() which was
attempting to find the relevant child partition index corresponding to the
given indexOid. As it turned out, thanks to -Wshadow=compatible-local,
this code was buggy as the code which was finding the child indexes
assigned those to a shadowed variable that directly went out of scope.
The code which thought it was looking at the List of child indexes was
always referencing an empty List.
On further investigation, this code is dead. We never call
CreateTriggerFiringOn() passing a valid indexOid in a way that the
function would actually ever execute the code in question. So, for lack
of a way to test if a fix actually works, let's just remove the dead code
instead.
As a reminder, if there is ever a need to resurrect this code, an Assert()
has been added to remind future feature developers that they might need to
write some code to find the corresponding child index.
Reported-by: Justin Pryzby
Reviewed-by: Justin Pryzby
Discussion: https://postgr.es/m/20220819211824.GX26426@telsasoft.com
In a similar effort to f736e188c and 110d81728, fixup various usages of
string functions where a more appropriate function is available and more
fit for purpose.
These changes include:
1. Use cstring_to_text_with_len() instead of cstring_to_text() when
working with a StringInfoData and the length can easily be obtained.
2. Use appendStringInfoString() instead of appendStringInfo() when no
formatting is required.
3. Use pstrdup(...) instead of psprintf("%s", ...)
4. Use pstrdup(...) instead of psprintf(...) (with no formatting)
5. Use appendPQExpBufferChar() instead of appendPQExpBufferStr() when the
length of the string being appended is 1.
6. appendStringInfoChar() instead of appendStringInfo() when no formatting
is required and string is 1 char long.
7. Use appendPQExpBufferStr(b, .) instead of appendPQExpBuffer(b, "%s", .)
8. Don't use pstrdup when it's fine to just point to the string constant.
I (David) did find other cases of #8 but opted to use #4 instead as I
wasn't certain enough that applying #8 was ok (e.g in hba.c)
Author: Ranier Vilela, David Rowley
Discussion: https://postgr.es/m/CAApHDvo2j2+RJBGhNtUz6BxabWWh2Jx16wMUMWKUjv70Ver1vg@mail.gmail.com
Since these macros just cast whatever you give them to the designated
output type, and many normal uses also cast the output type further, a
number of incorrect uses go undiscovered. The fixes in this patch
have been discovered by changing these macros to inline functions,
which is the subject of a future patch.
Reviewed-by: Aleksander Alekseev <aleksander@timescale.com>
Discussion: https://www.postgresql.org/message-id/flat/8528fb7e-0aa2-6b54-85fb-0c0886dbd6ed%40enterprisedb.com
XLogPageRead() can retry internally after a pread() system call has
succeeded, in the case of short reads, and page validation failures
while in standby mode (see commit 0668719801). Due to an oversight in
commit 3f1ce973, these cases could leave stale data in the internal
cache of xlogreader.c without marking it invalid. The main defense
against stale cached data on failure to read a page was in the error
handling path of the calling function ReadPageInternal(), but that
wasn't quite enough for errors handled internally by XLogPageRead()'s
retry loop if we then exited with XLREAD_WOULDBLOCK.
1. ReadPageInternal() now marks the cache invalid before calling the
page_read callback, by setting state->readLen to 0. It'll be set to
a non-zero value only after a successful read. It'll stay valid as
long as the caller requests data in the cached range.
2. XLogPageRead() no long performs internal retries while reading
ahead. While such retries should work, the general philosophy is
that we should give up prefetching if anything unusual happens so we
can handle it when recovery catches up, to reduce the complexity of
the system. Let's do that here too.
3. While here, a new function XLogReaderResetError() improves the
separation between xlogrecovery.c and xlogreader.c, where the former
previously clobbered the latter's internal error buffer directly.
The new function makes this more explicit, and also clears a related
flag, without which a standby would needlessly retry in the outer
function.
Thanks to Noah Misch for tracking down the conditions required for a
rare build farm failure in src/bin/pg_ctl/t/003_promote.pl, and
providing a reproducer.
Back-patch to 15.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20220807003627.GA4168930%40rfd.leadboat.com
The planner has to special-case indexes on boolean columns, because
what we need for an indexscan on such a column is a qual of the shape
of "boolvar = pseudoconstant". For plain bool constants, previous
simplification will have reduced this to "boolvar" or "NOT boolvar",
and we have to reverse that if we want to make an indexqual. There is
existing code to do so, but it only fires when the index's opfamily
is BOOL_BTREE_FAM_OID or BOOL_HASH_FAM_OID. Thus extension AMs, or
extension opclasses such as contrib/btree_gin, are out in the cold.
The reason for hard-wiring the set of relevant opfamilies was mostly
to avoid a catalog lookup in a hot code path. We can improve matters
while not taking much of a performance hit by relying on the
hard-wired set when the opfamily OID is visibly built-in, and only
checking the catalogs when dealing with an extension opfamily.
While here, rename IsBooleanOpfamily to IsBuiltinBooleanOpfamily
to remind future users of that macro of its limitations. At some
point we might want to make indxpath.c's improved version of the
test globally accessible, but it's not presently needed elsewhere.
Zongliang Quan and Tom Lane
Discussion: https://postgr.es/m/f293b91d-1d46-d386-b6bb-4b06ff5c667b@yeah.net
Several backend-side loops scanning one or more directories with
ReadDir() (WAL segment recycle/removal in xlog.c, backend-side directory
copy, temporary file removal, configuration file parsing, some logical
decoding logic and some pgtz stuff) already know the type of the entry
being scanned thanks to the dirent structure associated to the entry, on
platforms where we know about DT_REG, DT_DIR and DT_LNK to make the
difference between a regular file, a directory and a symbolic link.
Relying on the direct structure of an entry saves a few system calls to
stat() and lstat() in the loops updated here, shaving some code while on
it. The logic of the code remains the same, calling stat() or lstat()
depending on if it is necessary to look through symlinks.
Authors: Nathan Bossart, Bharath Rupireddy
Reviewed-by: Andres Freund, Thomas Munro, Michael Paquier
Discussion: https://postgr.es/m/CALj2ACV8n-J-f=yiLUOx2=HrQGPSOZM3nWzyQQvLPcccPXxEdg@mail.gmail.com
The reverts the following and makes some associated cleanups:
commit f79b803dc: Common SQL/JSON clauses
commit f4fb45d15: SQL/JSON constructors
commit 5f0adec25: Make STRING an unreserved_keyword.
commit 33a377608: IS JSON predicate
commit 1a36bc9db: SQL/JSON query functions
commit 606948b05: SQL JSON functions
commit 49082c2cc: RETURNING clause for JSON() and JSON_SCALAR()
commit 4e34747c8: JSON_TABLE
commit fadb48b00: PLAN clauses for JSON_TABLE
commit 2ef6f11b0: Reduce running time of jsonb_sqljson test
commit 14d3f24fa: Further improve jsonb_sqljson parallel test
commit a6baa4bad: Documentation for SQL/JSON features
commit b46bcf7a4: Improve readability of SQL/JSON documentation.
commit 112fdb352: Fix finalization for json_objectagg and friends
commit fcdb35c32: Fix transformJsonBehavior
commit 4cd8717af: Improve a couple of sql/json error messages
commit f7a605f63: Small cleanups in SQL/JSON code
commit 9c3d25e17: Fix JSON_OBJECTAGG uniquefying bug
commit a79153b7a: Claim SQL standard compliance for SQL/JSON features
commit a1e7616d6: Rework SQL/JSON documentation
commit 8d9f9634e: Fix errors in copyfuncs/equalfuncs support for JSON node types.
commit 3c633f32b: Only allow returning string types or bytea from json_serialize
commit 67b26703b: expression eval: Fix EEOP_JSON_CONSTRUCTOR and EEOP_JSONEXPR size.
The release notes are also adjusted.
Backpatch to release 15.
Discussion: https://postgr.es/m/40d2c882-bcac-19a9-754d-4299e1d87ac7@postgresql.org
During dumptuples() the call to writetuple() would pfree any non-null
tuple. This was quite wasteful as this happens just before we perform a
reset of the context which stores all of those tuples.
It seems to make sense to do a bit of a code refactor to make this work,
so here we just get rid of the writetuple function and adjust the WRITETUP
macro to call the state's writetup function. The WRITETUP usage in
mergeonerun() always has state->slabAllocatorUsed == true, so writetuple()
would never free the tuple or do any memory accounting. The only call
path that needs memory accounting done is in dumptuples(), so let's just
do it manually there.
In passing, let's get rid of the state->memtupcount-- code that counts the
memtupcount down to 0 one tuple at a time inside the loop. That seems to
be a rather inefficient way to set memtupcount to 0, so let's just zero it
after the loop instead.
Author: David Rowley
Discussion: https://postgr.es/m/CAApHDvqZXoDCyrfCzZJR0-xH+7_q+GgitcQiYXUjRani7h4j8Q@mail.gmail.com
get_database_list() failed to restore the caller's memory context,
instead leaving current context set to TopMemoryContext which is
how CommitTransactionCommand() leaves it. The callers both think
they are using short-lived contexts, for the express purpose of
not having to worry about cleaning up individual allocations.
The net effect therefore is that supposedly short-lived allocations
could accumulate indefinitely in the launcher's TopMemoryContext.
Although this has been broken for a long time, it seems we didn't
have any obvious memory leak here until v15's rearrangement of the
stats logic. I (tgl) am not entirely convinced that there's no
other leak at all, though, and we're surely at risk of adding one
in future back-patched fixes. So back-patch to all supported
branches, even though this may be only a latent bug in pre-v15.
Reid Thompson
Discussion: https://postgr.es/m/972a4e12b68b0f96db514777a150ceef7dcd2e0f.camel@crunchydata.com
Peter Eisentraut
Andres Freund
Amit Kapila
Peter Geoghegan
Jeff Davis
Alvaro Herrera
Etsuro Fujita
Michael Paquier
Tom Lane
David Rowley
Robert Haas
John Naylor
Noah Misch
Daniel Gustafsson
Thomas Munro
Andrew Dunstan