not indicate an error.
So I think it should be:
```
if (conn->addr == NULL && conn->naddr != 0)
```
Other than that v15 looked very good. It was checked on Linux and
MacOS including running it under sanitizer.
I will take a look at v16 now.
--
Best regards,
Aleksander Alekseev
ld && ninja -C build &&
PG_TEST_EXTRA=1 meson test -C build && ninja -C build coverage-html
```
I'm sharing this for the sake of completeness. I don't have a strong
opinion on whether we should bother with covering every new line of
code with tests.
Except for the named nitpicks v16 looks good to me.
--
Best regards,
Aleksander Alekseev
oh, this one is fine since store_conn_addrinfo() is going to fail
only when we are out of memory.
--
Best regards,
Aleksander Alekseev
ill better than not
> having the test at all, because it is run in CI.
Got it, thanks.
I guess I'm completely out of nitpicks then!
--
Best regards,
Aleksander Alekseev
lvherre.pgsql
--
Best regards,
Aleksander Alekseev
.
However there are only a few days left if we are going to do this
within March CF...
--
Best regards,
Aleksander Alekseev
sql/src/test/modules/test_resowner/sql/test_resowner.sql:
No such file
```
I wonder why the tests pass when using Meson.
--
Best regards,
Aleksander Alekseev
ere seems to be no urgency
> for that...
Good idea.
> + Previously it was required to stop the postmaster and VACUUM the
> database
> + in a single-user mode. There is no need to use a single-user mode
> anymore.
>
> I think we need to go further and actively warn against i
rk
with logical replication in a sane and convenient way.
Correct me if I'm wrong, but I got an impression that the patch tries
to accomplish just that.
(*) Which personally I believe would be a good change. Unlikely to
happen, though.
--
Best regards,
Aleksander Alekseev
This appears in vacuum_get_cutoffs(), which is called by vacuum and cluster,
> and the open transactions were already mentioned, so this is not the place
> for this change.
Fixed.
> v4-0002:
>
> - errhint("Stop the postmaster and vacuum that database in single-user
> mode.\
v;
```
This happens because the developers of this SBC choose the default
username "user", which I had no reason to change.
Test merely checks that we can distinguish a username "user" from the
USER keyword. Maybe it's worth replacing "user" with "system_us
ot to protect against all possible user
names but merely to reduce the likelihood of the problem. For this
particular test there is no difference which keyword to use for the
test. I realize this is a minor problem, however the fix is trivial
too.
--
Best regards,
Aleksander Alekseev
Hi,
> Whether we need to have a test for this at all is perhaps a
> more interesting argument.
This was my initial thought but since somebody put it there I assumed
this is a very important test.
Any objections if we remove the tests for "user"?
--
Best regards,
Aleksander Alekseev
Hi,
> On Wed, Apr 12, 2023 at 03:30:03PM +0300, Aleksander Alekseev wrote:
> > Any objections if we remove the tests for "user"?
>
> Based on some rather-recent experience in this area with
> COERCE_SQL_SYNTAX, the relationship between the SQL keywords and the
>
see.
Please let me know what you think about all this. I'm going to prepare
an updated patch for the next CF so I could use early feedback.
--
Best regards,
Aleksander Alekseev
> The Pluggable TOAST was rejected, but we have a lot of improvements
> based on changing the TOAST pointer structure.
Interestingly it looks like we ended up working on TOAST improvement
after all. I'm almost certain that we will have to modify TOAST
pointers to a certain degree in order to make it work. Hopefully it's
not going to be too invasive.
[1]: https://www.postgresql.org/docs/current/sql-createtype.html
--
Best regards,
Aleksander Alekseev
fies it in postgresql.conf as usual.
I realize this is a complicated problem to solve in a general case,
but it doesn't look like the proposed patch is the right solution for
the named problem.
--
Best regards,
Aleksander Alekseev
ly
an unreliable solution for a problem it is supposed to address. I
don't think we should encourage the users to build unreliable systems.
--
Best regards,
Aleksander Alekseev
this is the case perhaps we can reduce the scenario and
consider this simpler one:
1. The table is truncated
2. The DBMS is killed before making a checkpoint
3. We are in recovery and presumably see a pair of 0.5 Gb segments
Or can't we?
--
Best regards,
Aleksander Alekseev
#x27;t we?
Oh, I see. If the process will be killed this perhaps is not going to
happen. Whether this can happen if we pull the plug from the machine
is probably a design implementation of the particular filesystem and
whether it's journaled.
Hm...
--
Best regards,
Aleksander Alekseev
too. So it looks like the
recovery protocol worked as expected after all, at least in the
upcoming PG16 and this specific scenario.
Did I miss anything? If not, perhaps we should just update the comments a bit?
--
Best regards,
Aleksander Alekseev
>> pow(2048,3) > pow(2,32) - 1
True
```
Hopefully I didn't miss or misunderstood anything.
Thoughts?
--
Best regards,
Aleksander Alekseev
mately 4K slots per FSM page after all:
```
>>> 8*1024-24-4 - sum([pow(2,n) for n in range(0,12) ])
4069
```
--
Best regards,
Aleksander Alekseev
ble
+ */
+StaticAssertDecl(MAXPGPATH >= BGW_MAXLEN, "MAXPGPATH must be at least
equal to BGW_MAXLEN");
```
library_name, not function_name. Also I think the comment should be
more detailed, something like "prior to PG17 we used ... but since
PG17 ... which may cause problems if ...".
--
Best regards,
Aleksander Alekseev
>bgw_library_name
```
I'm pretty confident something went wrong with the parentheses in v2.
--
Best regards,
Aleksander Alekseev
ostgresql.org/wiki/Mailing_Lists#Email_etiquette_mechanics
--
Best regards,
Aleksander Alekseev
ill be more beneficial for the community in
the long term.
Thoughts?
[1]: https://commitfest.postgresql.org/43/3626/
--
Best regards,
Aleksander Alekseev
t [1]. I agree that we should focus on
refactoring TOAST pointers first. So I suggest we continue discussing
this in a corresponding thread and return to this one later.
[1]:
https://www.postgresql.org/message-id/CAJ7c6TPSvR2rKpoVX5TSXo_kMxXF%2B-SxLtrpPaMf907tX%3DnVCw%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
m
```
```
select count(*) from truncateme;
count
-
1048576
```
So I'm still unable to reproduce the described scenario, at least on PG16.
--
Best regards,
Aleksander Alekseev
scenario you initially
described is probably the wrong one for reasons yet to be understood
and we need to come up with a better one.
--
Best regards,
Aleksander Alekseev
();
PopActiveSnapshot();
CommitTransactionCommand();
... and also clarifies that the order of PushActiveSnapshot(...) and
SPI_connect() is not important.
--
Best regards,
Aleksander Alekseev
From 9f20a508c3b52d94455d67e3bdf7872787c63255 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev
Date
7;t change anything). As always, engineering is all about compromises.
It's up to the committer to decide.
[1]:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=frogfish&dt=2022-03-25%2018%3A37%3A10
--
Best regards,
Aleksander Alekseev
ut the other patches and it is OK. cfbot is happy
with the patchset as well.
--
Best regards,
Aleksander Alekseev
t me know
if I'm moving the right direction. There will be more tests in the
final version, but I would appreciate an early feedback.
[1]: https://postgr.es/m/220fab30-dff0-b055-f803-4338219f1021%40enterprisedb.com
--
Best regards,
Aleksander Alekseev
v1-0001-Unit-tests-for-SLRU.patch
Description: Binary data
yx9d6rh5JZr_tPESg%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
imagine how
someone may decide to use it in an extension. Wouldn't it be more logical
to place it near:
src/test/modules/test_rbtree
src/test/modules/test_shm_mq
... etc?
Again, I don't have a strong opinion here. If you insist, I will place the
tests to regress.c.
--
Best regards,
Aleksander Alekseev
code. I'm currently
working on improving this number.
More feedback is always welcome!
--
Best regards,
Aleksander Alekseev
v2-0001-Unit-tests-for-SLRU.patch
Description: Binary data
h from what I know is not exactly what
unit tests are for. We can make it public if we want to, but considering the
simplicity of the function and the existence of many other tests I didn't find
it necessary.
I think the tests are about as good as they will ever get.
--
Best regards,
Aleksa
Hi hackers,
> Here is version 3 of the patch.
> [...]
> I think the tests are about as good as they will ever get.
Here is version 4. Same as v3, but with resolved conflicts against the
current `master` branch.
--
Best regards,
Aleksander Alekseev
v4-0001-Unit-tests-for-S
OK with the accompanying limitations? Any
alternative suggestions?
- All in all, am I moving the right direction?
Your feedback is very much welcomed!
[1]:
https://postgr.es/m/CAJ7c6TPx7N-bVw0dZ1ASCDQKZJHhBYkT6w4HV1LzfS%2BUUTUfmA%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
v1-0001-CREATE-TYPE-foo-AS-DICTIONARY-OF-JSONB.patch
Description: Binary data
Hi hackers,
Many thanks for all your great feedback!
Please see the follow-up thread '[PATCH] Compression dictionaries for JSONB':
https://postgr.es/m/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22%3D5xVBg7S4vr5rQ%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
s some FIXME's
- The path includes some simple tests now
- A proper commit message was added
Please note that this is still a draft. Feedback is welcome.
--
Best regards,
Aleksander Alekseev
v2-0001-Compression-dictionaries-for-JSONB.patch
Description: Binary data
Hi,
> > LGTM, except for one small detail: I noticed that you misspelled
> > "translations" in the commit message.
>
> Oops. Fixed locally.
v11-0001 and v11-0002 LGTM too. IMO "to assign a XID" sounds better
than "to generate a XID".
--
Best regards,
Aleksander Alekseev
this is a good change.
Maybe we should also display the defaults for -X,
--manifest-checksums, etc for consistency.
--
Best regards,
Aleksander Alekseev
regards,
Aleksander Alekseev
commit 80df7cf26476a3ede42310354715e972fa40a8cf
Author: Aleksander Alekseev
Date: Thu Oct 19 16:30:27 2023 +0300
reproduce
diff --git a/src/test/regress/expected/event_trigger.out
b/src/test/regress/expected/event_trigger.out
index eaaff6ba6f..741099747c 1
while we have the SGML and man pages to do the job, with always more
> details.
Right. Then I suggest merging the patch as is.
--
Best regards,
Aleksander Alekseev
and proposals are welcome.
It seems to me that discarding the previous discussion and starting a
new thread where you ask the community for *another* feedback is not
going to be productive. Pretty sure it's not going to change.
--
Best regards,
Aleksander Alekseev
n top of that not sure if I see the patch on the November commitfest
[1]. Please make sure it's there so that cfbot will check the patch.
[1]: https://commitfest.postgresql.org/45/
--
Best regards,
Aleksander Alekseev
Hi,
> On Wed, 2023-10-25 at 16:17 +0300, Aleksander Alekseev wrote:
> > On top of that not sure if I see the patch on the November commitfest
> > [1]. Please make sure it's there so that cfbot will check the patch.
>
> Yes, this patch is listed on the November com
r12U37pzGEsU6Fg%40mail.gmail.com
[3]:
https://www.postgresql.org/message-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22%3D5xVBg7S4vr5rQ%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
OAST for JSON
and other types. We try to be mindful of use cases you named before
like 64-bit TOAST pointers but we still could use your input.
You know all this.
[1]:
https://www.postgresql.org/message-id/flat/CAJ7c6TOtAB0z1UrksvGTStNE-herK-43bj22%3D5xVBg7S4vr5rQ%40mail.gmail.com
--
Best re
directory
doesn't match '(?^:error: could not read permissions of directory)'
```
Should we simply use something like:
```
qr/error: could not (read|change).* directory/
```
... instead?
--
Best regards,
Aleksander Alekseev
Michael,
On Fri, Oct 27, 2023 at 9:52 AM Michael Paquier wrote:
>
> On Tue, May 23, 2023 at 01:47:52PM +0300, Aleksander Alekseev wrote:
> > That's me still talking to myself :)
>
> Let's be two then.
Many thanks for your feedback.
> +
> +It is co
Hi,
> PFA patch v4.
I didn't like the commit message. Here is the corrected patch. Sorry
for the noise.
--
Best regards,
Aleksander Alekseev
v5-0001-Clarify-the-38.10.10.-Shared-Memory-and-LWLocks-s.patch
Description: Binary data
In my
use cases not running TAP tests typically is not something I want . So
I would appreciate being warned with a long list of bright yellow
"SKIP" messages. If I really want to skip TAP tests these messages are
just informative and don't bother me.
+1
--
Best regards,
Aleksander Alekseev
> The result has been applied as fe705ef6fc1d.
Many thanks!
--
Best regards,
Aleksander Alekseev
ill have no complaints I suggest merging them.
--
Best regards,
Aleksander Alekseev
v4-0003-pg_resetwal-Add-more-tests-and-test-coverage.patch
Description: Binary data
v4-0001-More-consistent-behavior-of-GetDataDirectoryCreat.patch
Description: Binary data
v4-0002-doc-pg_resetwal-Add-comment
least for consistenc.
I see one more place with a similar code in guc.c around line 1472.
Although I don't have exact steps to trigger a crash I suggest adding
a similar check there.
--
Best regards,
Aleksander Alekseev
Hi,
> I didn't see any recent mentions in the archives, so I'll volunteer to
> be CF manager for 2023-11.
Many thanks for volunteering! If you need any help please let me know.
--
Best regards,
Aleksander Alekseev
trace. I would suggest adding debug output to
010_tab_completion.pl to figure out on which line the script fails.
Then I would figure out the exact command that failed. Then I would
execute it manually and compare the result with my expectations.
--
Best regards,
Aleksander Alekseev
e out-of-core code. This is one of these
> things where we could break something just for the sake of breaking
> it, so there is no real benefit IMO.
Fair enough, here is the corrected patch.
--
Best regards,
Aleksander Alekseev
v3-0001-Refactor-inval.c.patch
Description: Binary data
bject here [1].
I wish you all the best on the journey and look forward to your contributions!
[1]:
https://www.timescale.com/blog/how-and-why-to-become-a-postgresql-contributor/
--
Best regards,
Aleksander Alekseev
uggest keeping the
commit message as is or shuffling the authors randomly. I believe we
should do our best to reflect the input of people who previously
contributed to the effort, if anyone are aware of them, and add them
to the commit message if they are not there yet. Pretty sure Git will
forgive us if the list ends up being long. Hopefully so will people
who we mistakenly forget.
--
Best regards,
Aleksander Alekseev
didn't understand this one. Maybe you could provide the exact code?
--
Best regards,
Aleksander Alekseev
bers a good test.
Fixed. I choose not to change any numbers in the test in order to
check any corner cases, etc. The code patches for long_segment_names =
true and long_segment_names = false are almost the same thus it will
not improve code coverage. Using the current numbers will allow to
eas
length.
*
* XXX should we still consider such names to be valid?
*/
return (len == 4 || len == 5 || len == 6);
```
Should we just drop it or check that segno is <= 0xFF?
--
Best regards,
Aleksander Alekseev
; segno <= 0xFF);
return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
(unsigned int) segno);
}
--
Best regards,
Aleksander Alekseev
v60-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
De
om/bliki/TwoHardThings.html
--
Best regards,
Aleksander Alekseev
in_mutable_functions_after_planner_transformations(). This being
said, in practice one should read the comments to learn about corner
cases, pre- and postconditions anyway, so maybe it's not that a big
deal. I think of contain_mutable_functions_after_transformations() as
a good compromise
existing function names.
Fair enough.
--
Best regards,
Aleksander Alekseev
aparound we didn't document particular steps
to trigger XID wraparound. I don't recall encountering such examples
in the documentation, so I don't think we should add them here, at
least for consistency.
All in all the patch seems to be as good as it will get. I suggest merging it.
--
Best regards,
Aleksander Alekseev
`
Negative span ids, for sure, are possible, but I don't recall seeing
many negative ids in my life. I suggest not to surprise the user
unless this is really necessary.
--
Best regards,
Aleksander Alekseev
t; only in session backends?
Temporary tables, by definition, are visible only within one session.
I can't imagine how and why they would be replicated.
--
Best regards,
Aleksander Alekseev
tex_lock would be appropriate. Personally I am inclined to
think that the automatic test in this particular case is redundant.
[1]: https://www.postgresql.org/docs/current/libpq-threading.html
[2]: https://commitfest.postgresql.org/
[3]: http://cfbot.cputube.org/
--
Best regards,
Aleksander Alekseev
there is a point in doing a backpatch?
I disagree that it's cosmetic. The test doesn't check what it's supposed to.
--
Best regards,
Aleksander Alekseev
cts and the second one - with
Relation objects.
--
Best regards,
Aleksander Alekseev
setup --buildtype debug -DPG_TEST_EXTRA="kerberos ldap
ssl" -Dldap=disabled -Dssl=openssl -Dcassert=true -Dtap_tests=enabled
-Dprefix=/Users/eax/pginstall build && ninja -C build && ninja -C
build docs && meson test -C build'
```
I don't see any warnings when using Autotools.
--
Best regards,
Aleksander Alekseev
ified 1_000_000+
iteration this could also indicate a user error.
If we want to add CHECK_FOR_INTERRUPTS inside the loop I think a brief
comment would be appropriate.
--
Best regards,
Aleksander Alekseev
art treating them as regular tables this will cause a severe
performance degradation. I doubt that such a patch will make it.
I sort of suspect that you are working on a very specific extension
and/or feature for PG fork. Any chance you could give us more details
about the case?
--
Best regards,
Aleksan
at the new practical limit would be. Regardless of the
chosen number there is a possibility of breaking backward
compatibility for certain users.
For this reason I believe merging the proposed patch would be the
right move at this point. It doesn't make anything worse for the
existing users and s
tgresql.org/docs/current/sql-createdomain.html
--
Best regards,
Aleksander Alekseev
xible as a result.
Well even assuming this patch will make it to the upstream some day,
which I seriously doubt, it will take somewhere between 2 and 5 years.
Personally I would recommend reconsidering this design.
--
Best regards,
Aleksander Alekseev
ng term (i.e. always long_segment_names == true) could be a better
strategy. Maybe it's not but I believe this should be discussed
separately from the patchset under question.
--
Best regards,
Aleksander Alekseev
ed buffers as proposed
elsewhere [1].
[1]:
https://www.postgresql.org/message-id/efaac0be-27e9-4186-b925-79b7c696d...@amazon.com
--
Best regards,
Aleksander Alekseev
rnings and
also building the patch on Windows:
http://cfbot.cputube.org/
--
Best regards,
Aleksander Alekseev
urrent session
and/or a given session ID. In the latter case it can have an effect
only for the new transactions. This however could be implemented
separately from the proposed patchset. I suggest keeping the scope
small.
--
Best regards,
Aleksander Alekseev
difier requires Perl >= 5.14, which is fine [1].
[1]: https://www.postgresql.org/docs/current/install-requirements.html
--
Best regards,
Aleksander Alekseev
)
--
Best regards,
Aleksander Alekseev
Hi,
> least lists what's expected -- I'm not sure if this is the way to go,
> or if we should somehow list the individual tools that are failing
> here?
Personally I think the patch is OK.
--
Best regards,
Aleksander Alekseev
ollate, &src_ctype, &src_iculocale,
&src_icurules, &src_locprovider,
&src_collversion))
ereport(ERROR, ...
```
Should we just silence the warning like this - see attachment? I don't
think createdb() is called that often to worry about slight
performance change, if there is any.
--
Best regards,
Aleksander Alekseev
v1-0001-Silence-compiler-warnings.patch
Description: Binary data
fix the comment.
>
> sure, thanks for the explanation.
The patch was applied in 8bf7db02 [1] and I assume it's safe to close
the corresponding CF entry [2].
Thanks, everyone.
[1]:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8bf7db0285dfbc4b505c8be4c34ab7386eb6297f
[2]: https://commitfest.postgresql.org/44/4521/
--
Best regards,
Aleksander Alekseev
Hi,
> I am seeing a new gcc 12.2.0 compiler warning from
> src/backend/commands/sequence.c:
Yep, the compiler is just not smart enough to derive that this
actually is not going to happen.
Here is a proposed fix.
--
Best regards,
Aleksander Alekseev
v1-0001-Silence-GCC-12-warning
ST value ID" [1] is one of such "Waiting on author"
patches I've been reviewing. See the last 2-3 messages in the thread.
I believe it's safe to mark it as RwF for now.
[1]: https://commitfest.postgresql.org/44/4296/
--
Best regards,
Aleksander Alekseev
Rejected.
I will apply corresponding status changes if there will be no objections.
--
Best regards,
Aleksander Alekseev
the
best shape, so we have to prioritise. Please feel free re-submitting
the patch for the next commitfest.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
--
Best regards,
Aleksander Alekseev
eel free re-submitting
the patch for the next commitfest.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
--
Best regards,
Aleksander Alekseev
eel free re-submitting
the patch for the next commitfest.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
--
Best regards,
Aleksander Alekseev
tember CF a
consensus was reached [1] to mark this patch as Rejected.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
--
Best regards,
Aleksander Alekseev
in
good shape and previously were marked as "Ready for Committer".
Actually I thought Heikki would commit them to PG16, but it didn't
happen. If there are no objections, I will return the RfC status in a
bit since it seems to be more appropriate in this case.
--
Best regards,
Aleksander Alekseev
nd then for instance add my own
patches to the thread. In cases like this I end up being both an
author and a reviewer, but it doesn't mean that I review my own
patches :)
In this particular case IMO it would be appropriate to remove myself
from the list of reviewers. So I did.
Thanks!
--
Best regards,
Aleksander Alekseev
201 - 300 of 974 matches
Mail list logo
