FC. We did it in the past in similar cases and this
approach proved to be productive.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
[2]: https://commitfest.postgresql.org/44/3514/
[3]: http://cfbot.cputube.org/
[4]:
https://postgr.es/m/CAJ7c6TN%3D1EF1bTA6w8W%3D0e_Bj%2B-jsiHK0ap1uC_ZUGjwu%2B4JGw%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
/postgr.es/m/CAJ7c6TME5Z8k4undYUmKavD_dQFL0ujA%2BzFCK1eTH0_pzM%3DXrA%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
ming CF.
This being said, Peter is the CF manager, so he has every right to
change the status of the patches under questions if he disagrees.
--
Best regards,
Aleksander Alekseev
for the September CF [1] a consensus was
reached that the patchset needs another round of review. Also I
removed myself from the list of reviewers in order to make it clear
that a review from somebody else would be appreciated.
[1]: https://postgr.es/m/0737f444-59bb-ac1d-2753-873c40da0840%40eisentraut.org
--
Best regards,
Aleksander Alekseev
authors directly about the
> reasons why the patch is being revoked; so without "see consensus in
> [0]".
That's fair enough. I will use "It's been decided" or something like
this next time to avoid any confusion.
--
Best regards,
Aleksander Alekseev
dable (although it's shorter).
* XLogReadLength() - ditto
* `if (read < 0)` in DecodeXLogRecord() is noop since `read` is unsigned
--
Best regards,
Aleksander Alekseev
he fact that I
suggested to reject the patch and nobody objected straight away is not
a consensus.
> I think this patch is a good idea and should be committed.
No problem, changing status back to "Needs review".
--
Best regards,
Aleksander Alekseev
Hi,
> > I think this patch is a good idea and should be committed.
>
> No problem, changing status back to "Needs review".
Now when we continue reviewing the patch, could you please elaborate a
bit on why you think it's worth committing?
--
Best regards,
Aleksander Alekseev
s declared as a performance improvement. In such
cases we typically ask the authors to prove that the actual
improvement took place and that there were no degradations.
If we consider the patch marley as a refactoring that improves the
readability I see no reason not to merge it.
v2 LGTM.
--
Best regards,
Aleksander Alekseev
fferent test environments.
--
Best regards,
Aleksander Alekseev
esql.org/docs/current/xfunc-volatility.html
[3]:
https://postgr.es/m/CAJ7c6TMSat6qjPrrrK0tRTgZsdXwFAbkDn5gjeDtFnUFrjZX-g%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
. I added it to the nearest open
commitfest [1].
IMO a test is needed that makes sure no one is going to break this in
the future.
[1]: https://commitfest.postgresql.org/45/4559/
--
Best regards,
Aleksander Alekseev
7;t "WHEN it could be dangerous" be better?
```
+// FIXME: why 2?
if (set_oldest_commit_ts_xid < 2 &&
```
Should we rewrite this to < FrozenTransactionId ?
```
+$mult = 32 * $blcksz * 4; # FIXME
```
Unless I'm missing something this $mult value is not used. Is it
really needed here?
--
Best regards,
Aleksander Alekseev
nts before
every (or most) of the queries. Figuring out what they test could be
not particularly straightforward for somebody who will make changes
after the patch will be accepted.
[1]: http://cfbot.cputube.org/
[2]: https://github.com/afiskon/pgscripts/
--
Best regards,
Aleksander Alekseev
't have a strong opinion on
whether we should bother with a comment or not.
As a side note I wonder whether we shouldn't assume that query_buf is
always properly initialized elsewhere. But this is probably out of
scope of this particular discussion.
--
Best regards,
Aleksander Alekseev
Hi Michael,
> The patch looks incorrect to me. In case you've not noticed, we'd
> still have the same problem if do_edit() fails [...]
You are right, I missed it. Your patch is correct while the original
one is not quite so.
--
Best regards,
Aleksander Alekseev
_commit() which is confusing.
- pg_stat_get_db_last_commit_lsn() is marked as STABLE which is
probably correct. But I would appreciate a second opinion on this.
- Wouldn't it be appropriate to add a test or two?
- `if (!XLogRecPtrIsInvalid(commit_lsn))` - I suggest adding
XLogRecPtrIsValid() macro for better readability
--
Best regards,
Aleksander Alekseev
what exactly you want to profile - CPU, lock contention,
disk I/O, etc. People write books (plural) on the subject. Personally
I would recommend "System Performance, Enterprise and the Cloud, 2nd
Edition" and "BPF Performance Tools" by Brendan Gregg.
--
Best regards,
Aleksander Alekseev
om/playlist?list=PLSE8ODhjZXjYzlLMbX3cR0sxWnRM7CLFn
--
Best regards,
Aleksander Alekseev
atches 0001..0005 seem to be ready and rather independent. I
suggest merging them and continue discussing the rest of the patches.
--
Best regards,
Aleksander Alekseev
ar task, IMO. We don't use
rbtrees for everything that needs a map from x to y. Hash tables have
other compromises. Sometimes one container is a better fit, sometimes
the other, sometimes none and we implement a fine tuned container for
the given case. Same here.
--
Best regards,
Aleksander Alekseev
W, I was able to
> reproduce the assertion failure Kuwamura-san reported, even after applying
> your latest patch from the thread.
Do you mean that the test fails or it doesn't but there are other
steps to reproduce the issue?
--
Best regards,
Aleksander Alekseev
um, Page page, uint8 flags)
```
Will there be a value in addressing anything of this?
--
Best regards,
Aleksander Alekseev
Hi,
> On 03.10.23 13:28, Aleksander Alekseev wrote:
> > While examining the code for similar places I noticed that the
> > following functions can also be const'ified:
> >
> > - crc32_sz
>
> I suppose this could be changed.
OK, that's a simple c
to me off-list.
[1]: https://commitfest.postgresql.org/45/4128/
--
Best regards,
Aleksander Alekseev
eral environments,
not surprisingly. Let's merge it.
--
Best regards,
Aleksander Alekseev
PI in this particular aspect
doesn't strike me as such a terrible idea.
--
Best regards,
Aleksander Alekseev
{
+
Assert(bms_membership(root->all_baserels) == BMS_MULTIPLE);
return true;
+ }
}
+
+ Assert(bms_membership(root->all_baserels) != BMS_MULTIPLE);
return false;
}
```
It wasn't. The patch LGTM.
--
Best regards,
Aleksander Alekseev
e any of
this, although I'm not an expert in PostGIS.
All PostgreSQL GUCs are well documented. You can find more details here [1].
Hopefully I answered the right question. If not please be a bit more specific.
[1]: https://www.postgresql.org/docs/current/runtime-config-client.html
--
Best regards,
Aleksander Alekseev
s similarly to
how we test SLRU, background workers, etc.
[1]:
https://www.postgresql.org/message-id/flat/CAN-LCVMq2X%3Dfhx7KLxfeDyb3P%2BBXuCkHC0g%3D9GF%2BJD4izfVa0Q%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
Hi,
> Those all make sense to me.
>
> > [...]
>
> Of course. Your general approach seems wise.
>
> Thanks for working on this. I will be relieved once this is finally
> taken care of.
+1, and many thanks for your attention to the patchset and all the details!
-
extra_bits << 32) | t_xmax
... and save 8 extra bytes in the pd_special area. Or maybe I'm
missing some context here?
--
Best regards,
Aleksander Alekseev
Hi Tom,
> I looked at this and am inclined to reject it. [...]
OK, thanks. Then we are done with this thread. I closed the
corresponding CF entry.
--
Best regards,
Aleksander Alekseev
wdgw9ofewnxcjmmwl...@mail.gmail.com
--
Best regards,
Aleksander Alekseev
v1-0001-Constify-the-arguments-of-ilist.c-h-functions.patch
Description: Binary data
le who may be
interested in starting to contribute to PostgreSQL. Maybe I'll be able
to find a volunteer.
[1]:
https://www.postgresql.org/message-id/flat/CAJ7c6TM2=08mnkd9ajg8vey9hd+g4l7+nvh30uint3kshgr...@mail.gmail.com
--
Best regards,
Aleksander Alekseev
m. Or adding a ToDo item here https://wiki.postgresql.org/wiki/Todo
> might also help?
Good point. Will it be better to use the "Miscellaneous Other" section
for this or create a new "Code coverage" section?
--
Best regards,
Aleksander Alekseev
rd to v24!
This is a major change so I hope there will be more feedback from
other people on the mailing list.
--
Best regards,
Aleksander Alekseev
Hi Himanshu,
> Test cases are now part of this v6 patch.
I believe the patch is in pretty good shape now. I'm going to change
its status to "Ready for Committer" soon unless there are going to be
any objections.
--
Best regards,
Aleksander Alekseev
.] you'd better put this facility in src/test/modules/
Fair enough. PFA the corrected patch v5.
--
Best regards,
Aleksander Alekseev
v5-0001-Add-unit-tests-for-SLRU.patch
Description: Binary data
The patchset seems to be in very good shape except for these few
nitpicks. I'm inclined to change its status to "Ready for Committer"
as soon as the new version will pass cfbot unless there are going to
be any objections from the community.
--
Best regards,
Aleksander Alekseev
little more research and I think you are right. What happens
according to the C standard:
"""
the value is converted to unsigned by adding to it one greater than the largest
number that can be represented in the unsigned integer type
"""
so this is effectively -1 + (PG_UINT32_MAX + 1).
--
Best regards,
Aleksander Alekseev
zFtLYFpBoq_rKegW3On_8ZHdxB1mVv3-A%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
code. Other than that to me it looks ready to be
committed.
--
Best regards,
Aleksander Alekseev
v7-0001-Add-unit-tests-for-SLRU.patch
Description: Binary data
~5% penalty in terms of the
WAL size. This seems to be expected considering the fact that
sizeof(XLogRecord) became 4 bytes larger and the average record size
was about 66 bytes.
--
Best regards,
Aleksander Alekseev
rCRkZpnpeKQ9OenQ%40mail.gmail.com
[3]:
https://www.postgresql.org/message-id/CAJ7c6TN-N3%3DPSykmOjmW1EAf9YyyHFDHEznX-5VORsWUvVN-5w%40mail.gmail.com
[4]:
https://www.postgresql.org/message-id/CAJ7c6TO2XTTk3cu5w6ePHfhYQkoNpw7u1jeqHf%3DGwn%2BoWci8eA%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
v10-
t arguable perspective. It would be great
to notify the user about the potential issues with the configuration
and/or the fact that VACUUM doesn't catch up. But it doesn't mean we
should keep 32-bit XIDs in order to achive this.
--
Best regards,
Aleksander Alekseev
onsider starting a new thread.
This at least is my personal opinion. Let's give the rest of the
community a chance to share their thoughts.
--
Best regards,
Aleksander Alekseev
he notification Chris proposes should differ or
why it is in scope of this patch. If the point was to make sure
certain existing notifications will be preserved - sure, why not.
[1]:
https://www.postgresql.org/docs/current/routine-vacuuming.html#VACUUM-FOR-WRAPAROUND
--
Best regards,
Aleksander Alekseev
Hi Andres,
Thanks for the review!
> I don't think it is correct for any of these to add const. The only reason it
> works is because of casting etc.
Fair enough. PFA the corrected patch v2.
--
Best regards,
Aleksander Alekseev
v2-0001-Constify-the-arguments-of-ilist.c-h-func
lso monitoring xid lag,
> then by the time corrective action is taken it may be too late.
Good point but I still don't think this is related to the XID
wraparound problem.
--
Best regards,
Aleksander Alekseev
yet and thus
its tuples can't be frozen.
"""
If it is, please let me know. I would very much like to know if my
understanding here is flawed.
--
Best regards,
Aleksander Alekseev
nding Asserts().
[1]: https://github.com/afiskon/postgresql-extensions/tree/main/005-table-access
--
Best regards,
Aleksander Alekseev
v1-0001-Check-snapshot-argument-of-index_beginscan-and-fa.patch
Description: Binary data
ereport() but my intuition tells me that the
committers are not going to approve this :)
Let's see what the rest of the community thinks.
--
Best regards,
Aleksander Alekseev
TABLE foo IN ACCESS SHARE MODE;
LOCK TABLE bar IN ACCESS SHARE MODE;
do:
LOCK TABLE foo, bar, ... IN ACCESS SHARE MODE;
The proposed patch makes this change. It's pretty straightforward and
as a side effect saves a bit of network traffic too.
Thoughts?
--
Best regards,
Aleksander Alekseev
ll us more about your case and how the patch is going to help
with it. Also, maybe you could test your load with the applied
patchset and tell us whether it makes things better or worse?
Personally I would love hearing this from you.
--
Best regards,
Aleksander Alekseev
> There is an omission of automatic completion of CURRENT_ROLE in
> tab-complete.c.
I invested some time in checking this patch. It passes make
check-world / make installcheck-world and adds CURRENT_ROLE to the
automatic completion.
--
Best regards,
Aleksander Alekseev
tring then it defaults
> to NULL, which means the gss_krb5_ccache_name call is not made and the
> current behaviour (of letting the GSSAPI library work out the location
> of the ccache) is not changed.
>
> Many thanks,
> Daniel
>
--
Best regards,
Aleksander Alekseev
tried to search through the mailing list but
didn't find anything relevant. The complete script that reproduces the
issue is attached. I'm using the same script on Ubuntu VM, where it works
just fine.
[1]: https://github.com/rbenv/rbenv/issues/962#issuecomment-275858404
--
Best regards
Hi Tom,
Many thanks, running "make install" before "make check" helped.
On Tue, Apr 20, 2021 at 6:02 PM Tom Lane wrote:
>
> Aleksander Alekseev writes:
> > While trying to build PostgreSQL from source (master branch, 95c3a195) on a
> > MacBook I discovere
a VPN
> account. If the scenario above happens, there can exist a VPN account that
> does not have any presence in the central database and can be a security
> issue.
>
> I hope I explained it sufficiently. :-)
>
> Do you think, that would be possible to implement a process that would solve
> this use case?
>
> Thank you
> Ondrej
--
Best regards,
Aleksander Alekseev
ere commit on the master happened before receiving replies from the standby(s).
[1]:
https://www.postgresql.org/docs/13/runtime-config-wal.html#GUC-SYNCHRONOUS-COMMIT
--
Best regards,
Aleksander Alekseev
ty is
ready to accept a patch as is to make existing users suffer less and
iterate afterward. Or we choose not to do it and to come up with
another idea. Personally, I don't have any better ideas, thus maybe
accepting Andrey's patch would be the lesser of two evils.
--
Best regards,
Aleksander Alekseev
eck (x <> 5) not valid;
ALTER TABLE
eax=# alter table foo validate constraint ba
bar baz
eax=# alter table foo validate constraint bar;
ALTER TABLE
--
Best regards,
Aleksander Alekseev
v2-0001-tab-completion-of-ALTER-TABLE.VALIDATE-CONSTRAINT.patch
Description: Binary data
Hi hackers,
> Otherwise the patch passes all the tests and works as expected
I've noticed there is no tab completion for ALTER TABLE xxx ADD. Here
is an alternative version of the patch that fixes this as well. Not
sure if this should be in the same commit though.
--
Best regards,
Al
e next major version.
If we receive a complaint during beta-testing we can revert the patch
or maybe increase the limit for small messages.
--
Best regards,
Aleksander Alekseev
Hi Tom,
> scratches head ... ] The patch still applies perfectly cleanly
> for me, using either "patch" or "git apply".
Sorry, my bad. It was about lines separating on different platforms. The
patch is fine and passes installcheck-world on MacOS.
--
Best regards,
Aleksander Alekseev
eone could do it, it would be nice.
[1]: https://github.com/timescale/timescaledb/issues/1655
--
Best regards,
Aleksander Alekseev
ttps://github.com/afiskon/pgscripts
--
Best regards,
Aleksander Alekseev
leksander,
>
> On Wed, Apr 28, 2021 at 01:19:36PM +0300, Aleksander Alekseev wrote:
> > Hi Julien,
> >
> > > I'm attaching a patch that fixes those, with regression tests to
> reproduce each
> > > problem.
> >
> > I believe someth
the release notes.
--
Best regards,
Aleksander Alekseev
v2-0001-costumscan_projection_flag.patch
Description: Binary data
ed (or before) will be very
useful.
The patch applies to `master` branch (6d177e28) and passes all the
tests on MacOS.
--
Best regards,
Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation:tested, failed
This patch looks good to me. Considering a positive response
a while. The patch is fine.
> The new status of this patch is: Ready for Committer
--
Best regards,
Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation:tested, passed
This patch looks fine. Tested on MacOS Catalina; master 09ae3
issed
test_integerset.c. Here is an updated patch.
--
Best regards,
Aleksander Alekseev
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 2c2f149fb0..146b524076 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -1188,7 +1188,7 @@ file_acquire_samp
Sorry for a duplicate entry on CF web application
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation:tested, passed
Although the patch looks OK I would like to keep the status "
may consider adding ZSON to /contrib/.
If this is the case I will add this thread to the nearest CF and submit a
corresponding patch.
[1]: https://github.com/postgrespro/zson
--
Best regards,
Aleksander Alekseev
Open-Source PostgreSQL Contributor at Timescale
7;t
> alone.
Indeed, this is an extremely simple extension, ~500 effective lines of
code in C. It addresses a somewhat specific scenario, which, to my
regret, doesn't seem to be uncommon. A pain-killer of a sort. In an
ideal world, people suppose simply to normalize their data.
--
Best regards,
Aleksander Alekseev
themselves).
Ditto:
```
=# select '0x' || to_hex(x) from ( values (123), (-123) ) as s(x);
?column?
----
0x7b
0xff85
```
[1]: https://www.postgresql.org/docs/current/functions-string.html
--
Best regards,
Aleksander Alekseev
values (-2147483648) ) as s(x);
>
> ERROR: integer out of range
I'm afraid the behavior of something like to_hex(X, with_sign => true)
is going to be exactly the same. There is no safe and consistent way
to calculate abs(INT_MIN).
--
Best regards,
Aleksander Alekseev
Hi Dean,
> Of course there is. This is easy to code in C using unsigned ints,
> without resorting to abs() (yes, I'm aware that abs() is undefined for
> INT_MIN).
So in your opinion what is the expected result of to_hex(INT_MIN,
with_sign => true)?
--
Best regards,
Aleksander Alekseev
a bug [1] and I agree.
The proposed patch fixes this.
[1]: https://www.postgresql.org/message-id/22438.1477265185%40sss.pgh.pa.us
--
Best regards,
Aleksander Alekseev
v1-0001-Make-ON-CONFLICT-DO-NOTHING-and-ON-CONFLICT-DO-UP.patch
Description: Binary data
rouble for some
> of the uses of DO NOTHING out there.
Just to make sure we are on the same page. The patch doesn't break the
current DO NOTHING behavior but rather makes DO UPDATE work the same
way DO NOTHING does.
--
Best regards,
Aleksander Alekseev
s is to execute
several INSERTs one by one which is expensive when you have a lot of
INSERTs.
--
Best regards,
Aleksander Alekseev
Personally I would
prefer to get an ERROR in this case. And this is exactly how you end
up with even more flags.
I believe it would be better to let the user write the exact query
depending on what he/she wants.
--
Best regards,
Aleksander Alekseev
triggered: new = (2,1), old =
NOTICE: t_update triggered: new = (2,0), old = (2,1)
NOTICE: t_insert triggered: new = (3,1), old =
=# SELECT * FROM t;
a | b
---+---
1 | 0
2 | 0
3 | 1
```
PFA patch v2 that also includes the test shown above.
Are there any other scenarios we should check?
--
afiskon/pgscripts/
[2]: https://www.postgresql.org/docs/15/docguide-toolsets.html
--
Best regards,
Aleksander Alekseev
h for the documentation in a bit, after I'll check
it properly.
--
Best regards,
Aleksander Alekseev
, after I'll check
> it properly.
PFA the patch.
I don't have a strong opinion regarding any particular wording and
would like to ask the committer to change it as he sees fit.
--
Best regards,
Aleksander Alekseev
v1-0001-Document-the-workaround-for-xsltproc-when-buildin.patch
Description: Binary data
s successfully. The tests don't show that the checks
will fail if the indexes are corrupted. Usually we check this as well,
see bd807be6 and other amcheck replated patches and commits.
--
Best regards,
Aleksander Alekseev
c/xml/catalog
```
... work for you? Does your:
```
xsltproc --help
```
... also say that it uses /etc/xml/catalog path by default?
--
Best regards,
Aleksander Alekseev
```
brew uninstall --ignore-dependencies libxml2 libxslt docbook docbook-xsl
```
... right now. However maybe we should rephrase this to make sure
there are fewer supported/recommended ways of building the
documentation? The alternative ways may also work but if they don't
there will be no a
I'll take another fresh look and let you know my findings in a bit.
--
Best regards,
Aleksander Alekseev
> Of course, I'll take another fresh look and let you know my findings in a bit.
The code is well written, documented and test-covered. All the tests
pass. To my knowledge there are no open questions left. I think the
patch is as good as it will ever get.
--
Best regards,
Aleksander Alekseev
should
> be enough to pull in libxml2 as well.
Fair enough.
> 4. Use of --nonet changes the error message you get if xsltproc
> can't find the DTDs. I copied the error I get from MacPorts'
> version of xsltproc, but can you confirm it's the same on Homebrew?
Yes, the message is the same.
--
Best regards,
Aleksander Alekseev
rmally the ninja version that's pulled in by meson should suffice.
There are several ways to install Meson one of which, if you want the
latest version, is just using PIP:
```
pip3 install --user meson
```
Naturally Ninja will not be pulled in this case.
--
Best regards,
Aleksander Alekseev
ttps://postgr.es/m/CAJ7c6TOk1mx4KfF0AHkvXi%2BpkdjFqwTwvRE-JmdczZMAYnRQ0w%40mail.gmail.com
[3]:
https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2023_Developer_Meeting#v16_Patch_Triage
--
Best regards,
Aleksander Alekseev
ome time into reviewing
type-aware TOASTers :) I just choose to keep my personal opinion about
the complexity of that patch to myself this time since obviously I'm a
bit biased. However if you are curious it's all in the corresponding
thread.
--
Best regards,
Aleksander Alekseev
27;t seem to consider (3). Even after it
was explicitly pointed out that we should take a step back and return
to (1).
--
Best regards,
Aleksander Alekseev
301 - 400 of 974 matches
Mail list logo
