forked from postgres/postgres
-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow declaration after statement and reformat code to use it #2
Draft
JelteF
wants to merge
6
commits into
master
Choose a base branch
from
remove-useless-lines
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's an annoying edge case in C, where a declaration cannot be the first line after a label. A label needs to point to an expression, and a declaration is not an expression. There were only 4 cases where this was a problem in the entire codebase. I fixed those cases manually here in a way that the script does not autofix them back.
JelteF
force-pushed
the
remove-useless-lines
branch
from
August 19, 2021 10:49
18267e0
to
67f4373
Compare
JelteF
changed the title
Allow declaration after statement
Allow declaration after statement and reformat code to use it
Aug 19, 2021
JelteF
pushed a commit
that referenced
this pull request
Mar 31, 2022
Currently the pc files use hard coded paths for "includedir" and "libdir." Example: Cflags: -I/usr/include Libs: -L/usr/lib -lpq This is not very fortunate when cross compiling inside a buildroot, where the includes and libs are inside a staging directory, because this introduces host paths into the build: checking for pkg-config... /builder/shared-workdir/build/sdk/staging_dir/host/bin/pkg-config checking for PostgreSQL libraries via pkg_config... -L/usr/lib <---- This commit addresses this by doing the following two things: 1. Instead of hard coding the paths in "Cflags" and "Libs" "${includedir}" and "${libdir}" are used. Note: these variables can be overriden on the pkg-config command line ("--define-variable=libdir=/some/path"). 2. Add the variables "prefix" and "exec_prefix". If "includedir" and/or "libdir" are using these then construct them accordingly. This is done because buildroots (for instance OpenWrt) tend to rename the real pkg-config and call it indirectly from a script that sets "prefix", "exec_prefix" and "bindir", like so: pkg-config.real --define-variable=prefix=${STAGING_PREFIX} \ --define-variable=exec_prefix=${STAGING_PREFIX} \ --define-variable=bindir=${STAGING_PREFIX}/bin $@ Example #1: user calls ./configure with "--libdir=/some/lib" and "--includedir=/some/include": prefix=/usr/local/pgsql exec_prefix=${prefix} libdir=/some/lib includedir=/some/include Name: libpq Description: PostgreSQL libpq library Url: http://www.postgresql.org/ Version: 12.1 Requires: Requires.private: Cflags: -I${includedir} Libs: -L${libdir} -lpq Libs.private: -lcrypt -lm Example #2: user calls ./configure with no arguments: prefix=/usr/local/pgsql exec_prefix=${prefix} libdir=${exec_prefix}/lib includedir=${prefix}/include Name: libpq Description: PostgreSQL libpq library Url: http://www.postgresql.org/ Version: 12.1 Requires: Requires.private: Cflags: -I${includedir} Libs: -L${libdir} -lpq Libs.private: -lcrypt -lm Like this the paths can be forced into the staging directory when using a buildroot setup: checking for pkg-config... /home/sk/tmp/openwrt/staging_dir/host/bin/pkg-config checking for PostgreSQL libraries via pkg_config... -L/home/sk/tmp/openwrt/staging_dir/target-mips_24kc_musl/usr/lib Author: Sebastian Kemper <sebastian_ml@gmx.net> Co-authored-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com> Discussion: https://www.postgresql.org/message-id/flat/20200305213827.GA25135%40darth.lan
JelteF
pushed a commit
that referenced
this pull request
Jun 24, 2022
Due to how pg_size_pretty(bigint) was implemented, it's possible that when given a negative number of bytes that the returning value would not match the equivalent positive return value when given the equivalent positive number of bytes. This was due to two separate issues. 1. The function used bit shifting to convert the number of bytes into larger units. The rounding performed by bit shifting is not the same as dividing. For example -3 >> 1 = -2, but -3 / 2 = -1. These two operations are only equivalent with positive numbers. 2. The half_rounded() macro rounded towards positive infinity. This meant that negative numbers rounded towards zero and positive numbers rounded away from zero. Here we fix #1 by dividing the values instead of bit shifting. We fix #2 by adjusting the half_rounded macro always to round away from zero. Additionally, adjust the pg_size_pretty(numeric) function to be more explicit that it's using division rather than bit shifting. A casual observer might have believed bit shifting was used due to a static function being named numeric_shift_right. However, that function was calculating the divisor from the number of bits and performed division. Here we make that more clear. This change is just cosmetic and does not affect the return value of the numeric version of the function. Here we also add a set of regression tests both versions of pg_size_pretty() which test the values directly before and after the function switches to the next unit. This bug was introduced in 8a1fab3. Prior to that negative values were always displayed in bytes. Author: Dean Rasheed, David Rowley Discussion: https://postgr.es/m/CAEZATCXnNW4HsmZnxhfezR5FuiGgp+mkY4AzcL5eRGO4fuadWg@mail.gmail.com Backpatch-through: 9.6, where the bug was introduced.
JelteF
pushed a commit
that referenced
this pull request
Aug 18, 2022
We've heard a couple of reports of people having trouble with multi-gigabyte-sized query-texts files. It occurred to me that on 32-bit platforms, there could be an issue with integer overflow of calculations associated with the total query text size. Address that with several changes: 1. Limit pg_stat_statements.max to INT_MAX / 2 not INT_MAX. The hashtable code will bound it to that anyway unless "long" is 64 bits. We still need overflow guards on its use, but this helps. 2. Add a check to prevent extending the query-texts file to more than MaxAllocHugeSize. If it got that big, qtext_load_file would certainly fail, so there's not much point in allowing it. Without this, we'd need to consider whether extent, query_offset, and related variables shouldn't be off_t not size_t. 3. Adjust the comparisons in need_gc_qtexts() to be done in 64-bit arithmetic on all platforms. It appears possible that under duress those multiplications could overflow 32 bits, yielding a false conclusion that we need to garbage-collect the texts file, which could lead to repeatedly garbage-collecting after every hash table insertion. Per report from Bruno da Silva. I'm not convinced that these issues fully explain his problem; there may be some other bug that's contributing to the query-texts file becoming so large in the first place. But it did get that big, so #2 is a reasonable defense, and #3 could explain the reported performance difficulties. (See also commit 8bbe4cb, which addressed some related bugs. The second Discussion: link is the thread that led up to that.) This issue is old, and is primarily a problem for old platforms, so back-patch. Discussion: https://postgr.es/m/CAB+Nuk93fL1Q9eLOCotvLP07g7RAv4vbdrkm0cVQohDVMpAb9A@mail.gmail.com Discussion: https://postgr.es/m/5601D354.5000703@BlueTreble.com
JelteF
pushed a commit
that referenced
this pull request
Aug 25, 2022
Remove four probes for members of sockaddr_storage. Keep only the probe for sockaddr's sa_len, which is enough for our two remaining places that know about _len fields: 1. ifaddr.c needs to know if sockaddr has sa_len to understand the result of ioctl(SIOCGIFCONF). Only AIX is still using the relevant code today, but it seems like a good idea to keep it compilable on Linux. 2. ip.c was testing for presence of ss_len to decide whether to fill in sun_len in our getaddrinfo_unix() function. It's just as good to test for sa_len. If you have one, you have them all. (The code in #2 isn't actually needed at all on several OSes I checked since modern versions ignore sa_len on input to system calls. Proving that's the case for all relevant OSes is left for another day, but wouldn't get rid of that last probe anyway if we still want it for #1.) Discussion: https://postgr.es/m/CA%2BhUKGJJjF2AqdU_Aug5n2MAc1gr%3DGykNjVBZq%2Bd6Jrcp3Dyvg%40mail.gmail.com
JelteF
pushed a commit
that referenced
this pull request
Aug 25, 2022
We've heard a couple of reports of people having trouble with multi-gigabyte-sized query-texts files. It occurred to me that on 32-bit platforms, there could be an issue with integer overflow of calculations associated with the total query text size. Address that with several changes: 1. Limit pg_stat_statements.max to INT_MAX / 2 not INT_MAX. The hashtable code will bound it to that anyway unless "long" is 64 bits. We still need overflow guards on its use, but this helps. 2. Add a check to prevent extending the query-texts file to more than MaxAllocHugeSize. If it got that big, qtext_load_file would certainly fail, so there's not much point in allowing it. Without this, we'd need to consider whether extent, query_offset, and related variables shouldn't be off_t not size_t. 3. Adjust the comparisons in need_gc_qtexts() to be done in 64-bit arithmetic on all platforms. It appears possible that under duress those multiplications could overflow 32 bits, yielding a false conclusion that we need to garbage-collect the texts file, which could lead to repeatedly garbage-collecting after every hash table insertion. Per report from Bruno da Silva. I'm not convinced that these issues fully explain his problem; there may be some other bug that's contributing to the query-texts file becoming so large in the first place. But it did get that big, so #2 is a reasonable defense, and #3 could explain the reported performance difficulties. (See also commit 8bbe4cb, which addressed some related bugs. The second Discussion: link is the thread that led up to that.) This issue is old, and is primarily a problem for old platforms, so back-patch. Discussion: https://postgr.es/m/CAB+Nuk93fL1Q9eLOCotvLP07g7RAv4vbdrkm0cVQohDVMpAb9A@mail.gmail.com Discussion: https://postgr.es/m/5601D354.5000703@BlueTreble.com
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this?
It's a draft patch that replaces code like this:
With this shorter version:
Why would we want this?
a. This limits variable scope to what is necessary. Which in turn makes the mental model you have to keep of a function when reading the code simpler.
b. In most cases it allows you to see the the type of a variable without needing to go to the top of the function.
How was this changeset created?
I created a script that modifies all C files in the following way:
static
declaration.How does this script work?
It uses a Perl regex to search and replace! (obligatory jokes at the bottom of the email) This regex is based on the ones used in this PR to citus[4] and the similar PR to pg_auto_failover[5]. The script is in the 3rd commit of this patchset.
To visualize the regex in the script in a reasonable way, copy paste the search part of it:
And paste that into https://www.debuggex.com/, then select PCRE from the dropdown. (Sharing seems to be down at this point, so this is the only way to do it at the moment) Try it out! The regex is not as crazy as it looks.
There's two important assumptions this regex makes:
pgindent
)There's two cases where this regex has some special behaviour:
goto
or a preprocessor command is found (outside a string or comment). These things affect the control flow in a way that the regex does not understand. (any#
character is assumed to be a preprocessor commands).label:
, by checking if there was a:
as the last character on the previous line. This works around errors like this:Detecting this case in this way is not perfect, because sometimes there is an extra newline or a comment between the label and the assignment. This is not easily detectable by the regex, because lookbehinds cannot have a variable length in Perl (or most regex engines for that matter). For these few cases (5 in the entire codebase) a manual change was done either before or after the automatic replacement to fix them so the code compiles again. (2nd and 5th commits of this patchset)
After all these changes
make -s world
doesn't show any warnings or errors andmake check-world
succeeds. I configured compilation like this:What I want to achieve with this email
What I don't want to achieve with this email
Known issues with the currently generated code
There's a few issues with the final generated code that I've already spotted. These should all be relatively easy to fix in an automated way. However, I think doing so is probably better done by
pgindent
or some other auto formatting tool, instead of with the regex. Note that I did runpgindent
, it just didn't address these things:pgindent
adds a newline after each block of declarations, even if they are not at the start of function. If this is desired is debatable, but to me it seems like it adds excessive newlines. See [7] for an example.Sources:
tokei
[8] results of lines of code:Before:
After:
Obligatory jokes: