d new Asserts() make a
huge difference. I merely noticed that InitCatalogCache checks only
cacheinfo[cacheId].reloid. Checking the rest of the values would be
helpful for the developers and will not impact the performance of the
release builds.
--
Best regards,
Aleksander Alekseev
ty thinks.
[1] https://commitfest.postgresql.org/44/4456/
--
Best regards,
Aleksander Alekseev
Hi,
Currently InitCatalogCache() has only one Assert() for cacheinfo[]
that checks .reloid. The proposed patch adds sanity checks for the
rest of the fields.
--
Best regards,
Aleksander Alekseev
v1-0001-Check-more-invariants-during-syscache-initializat.patch
Description: Binary data
practical
value seems to be debatable.
The patch was added to the nearest commitfest [1].
Thoughts?
[1]: https://commitfest.postgresql.org/44/4451/
--
Best regards,
Aleksander Alekseev
focusing on delivering this
part, assuming there will be no push-back to the refactorings and
slight test improvements. If 0002 could be further decomposed into
separate iterative improvements this could be helpful.
--
Best regards,
Aleksander Alekseev
t regards,
Aleksander Alekseev
igger for REINDEX so far.
--
Best regards,
Aleksander Alekseev
100% sure but it looks like that's all of them. PFA the
> updated patch v2.
Added a CF entry, just in case:
https://commitfest.postgresql.org/44/4448/
--
Best regards,
Aleksander Alekseev
m. PFA the
updated patch v2.
--
Best regards,
Aleksander Alekseev
v2-0001-Pass-Datum-to-SearchSysCache-instead-of-Oid.patch
Description: Binary data
ps://commitfest.postgresql.org/44/4447/
--
Best regards,
Aleksander Alekseev
v2-0002-Rename-OverrideSearchPath-to-SearchPathMatcher.patch
Description: Binary data
v2-0001-Remove-unused-search_path-override-stack.patch
Description: Binary data
mindful that your functions will become a target for privilege
escalation, so you should be extra careful with the implementation.
[1]: https://www.postgresql.org/docs/current/sql-createfunction.html
--
Best regards,
Aleksander Alekseev
Hi,
> I have noticed 11 callers of SearchSysCache*() that pass down
> an Oid instead of a Datum.
Good catch.
> I think that we'd better be consistent here, even if there is
> no actual bug.
>
+1
--
Best regards,
Aleksander Alekseev
0... If there is anything that may help merging it into PG17 please
let me know.
--
Best regards,
Aleksander Alekseev
v26-0001-Make-End-Of-Recovery-error-less-scary.patch
Description: Binary data
this
particular implementation was chosen.
I added corresponding comments to SetLatchV() and SetLatches(). Also
the patchset needed a rebase. PFA v4.
It passes `make installcheck-world` on 3 machines of mine: MacOS x64,
Linux x64 and Linux RISC-V.
--
Best reg
east, one should prove
that this particular place is a bottleneck under given loads. I doubt
it is. Most of the time it's a network, disk I/O or locks.
So unless the author can provide benchmarks that show measurable
benefits of the change I suggest rejecting it.
--
Best regards,
Aleksander Alekseev
ixed
> here.
Unfortunately the patchset rotted quite a bit since February and needs a rebase.
--
Best regards,
Aleksander Alekseev
viewed in the scope of
July commitfest?
--
Best regards,
Aleksander Alekseev
essary.
--
Best regards,
Aleksander Alekseev
Hi,
> > Thanks, pushed after correcting a couple typos.
>
> Thanks!
I noticed that ec99d6e9c87a introduced a slight typo:
s/if there is not room/if there is no room
--
Best regards,
Aleksander Alekseev
means
+ * that in the future we can't decrease SLRU_PAGES_PER_SEGMENT easily.
+ */
+return snprintf(path, MAXPGPATH, "%s/%015llX", ctl->Dir,
(long long) segno);
else
return snprintf(path, MAXPGPATH, "%s/%04X", (ctl)->Dir,
```
Hi,
> Thanks, here's a fixed version. (ResourceOwner resource release
> callbacks mustn't call ResourceOwnerForget anymore, but AbortBufferIO
> was still doing that)
I believe v15 is ready to be merged. I suggest we merge it early in
the PG17 release cycle.
--
Best regards,
Aleksander Alekseev
t to proceed with.
Yes, this thread awaits several other patches to be merged [1] in
order to continue, so it makes sense to mark it as RwF for the time
being. Thanks!
[1]: https://commitfest.postgresql.org/43/3489/
--
Best regards,
Aleksander Alekseev
ider joining the compression
> dictionaries effort instead [1]. During the discussion with the
> community it ended up being a TOAST improvement after all. So we could
> use your expertise in this area.
>
> [1]: https://commitfest.postgresql.org/43/3626/
--
Best regards,
Aleksander Alekseev
[1]: https://commitfest.postgresql.org/43/3626/
--
Best regards,
Aleksander Alekseev
e problem in respect of naptime
Julien is referring to. If you know what this problem is and how to
fix it, go for it. I'll review and test the code then. I can write the
part of the patch that fixes the part regarding dynamic workers if
necessary.
--
Best regards,
Aleksander Alekseev
www.postgresql.org/message-id/flat/CAM-w4HPjg7NwEWBtXn1empgAg3fqJHifHo_nhgqFWopiYaNxYg%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
reverse ordering just give folks the opposite
> impression?
This is the exact reason why the original patch had an explicit
comment that the ordering is not important in this case. It was argued
however that the comment is redundant and thus it was removed.
--
Best regards,
Aleksander Alekseev
Hi Michael,
Thanks for your feedback.
> + * The order of PushActiveSnapshot() and SPI_connect() is not really
> + * important.
>
> FWIW, looking at the patch, I don't think that this is particularly
> useful.
Fair enough, here is the corrected patch.
--
Best rega
e beforehand, or indeed rely on TAP
> tests. Most projects properly setup their instance in the CI jobs, and at
> least the Debian packaging infrastructure has a way to configure it too.
Many thanks!
--
Best regards,
Aleksander Alekseev
l to make a complicated one out of it.
The order of the calls it currently uses however may be extremely
confusing for newcomers. It creates an impression that this particular
order is extremely important while in fact it's not and it takes time
to figure this out.
--
Best regards,
Aleksander Alekseev
ensions/tree/temp_config_experiment/007-gucs
--
Best regards,
Aleksander Alekseev
aps we should just say that the extension shouldn't be used
without shared_preload_libraies. We are not testing whether it works
in such a case anyway.
--
Best regards,
Aleksander Alekseev
es that the order of PushActiveSnapshot(...) and
> SPI_connect() is not important.
Additionally I noticed that the check:
```
if (!process_shared_preload_libraries_in_progress)
return;
```
... was misplaced in _PG_init(). Here is the patch v2 which fixes this too.
is sizeof(varatt_external) + /* header size */ 2, which seems
to be not extremely useful anyway. If you are interested in the topic
please consider joining the thread.
[1]:
https://postgr.es/m/CAN-LCVMq2X%3Dfhx7KLxfeDyb3P%2BBXuCkHC0g%3D9GF%2BJD4izfVa0Q%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
Hi,
> [...]
> A-ha, it's because I didn't add the new test_resowner directory to the
> src/test/modules/meson.build file. Fixed.
>
> Thanks for the review!
You probably already noticed, but for the record: cfbot seems to be
extremely unhappy with the patch.
--
Best regards,
Aleksander Alekseev
Hi hackers,
That's me still talking to myself :)
> Thoughts?
Evidently this works differently from what I initially thought on
Windows due to lack of fork() on this system.
PFA the patch v3. Your feedback is most welcomed.
--
Best regards,
Aleksander Alekseev
v3-0001-Clarify-the-38.10
tat_statements/regress/log/postmaster.log
--
Best regards,
Aleksander Alekseev
v2-0001-Clarify-the-usage-of-shmem_startup_hook.patch
Description: Binary data
https://commitfest.postgresql.org/43/4296/
--
Best regards,
Aleksander Alekseev
Hi,
> Unless I missed something, I suggest updating the documentation and
> pg_stat_statements.c accordingly.
Since no one seems to object so far I prepared the patch.
--
Best regards,
Aleksander Alekseev
v1-0001-Clarify-the-usage-of-shmem_startup_hook.patch
Description: Binary data
uot;virtual methods" will produce a cleaner
code but will also break branch prediction, so I don't think we should
use those.)
I don't think we need an example of adding a new TOAST tag in scope of
this work since the default one is going to end up being such an
example.
Does it make sense?
--
Best regards,
Aleksander Alekseev
LWLock* in local process memory instead of shared
memory is safe.
Unless I missed something, I suggest updating the documentation and
pg_stat_statements.c accordingly.
[1]:
https://github.com/afiskon/postgresql-extensions/blob/main/006-shared-memory/experiment.c#L38
--
Best regards,
Aleksander Alekseev
earn more about the internals of this part of the system
and accompanying edge cases? Perhaps there is an experiment or two an
extension author can do in order to lower the entry threshold and/or
known bugs, limitations or wanted features one could start with?
--
Best regards,
Aleksander Alekseev
memory will be inherited from postmaster by child processes and the
overall memory usage is going to be the same due to copy-on-write.
Perhaps we should clarify this.
Thoughts?
[1]: https://www.postgresql.org/docs/15/xfunc-c.html#XFUNC-SHARED-ADDIN
--
Best regards,
Aleksander Alekseev
estion
and/or familiar with Java.
I think you should address the question to pgsql-general@ mailing list
or StackOverflow.
(If you believe there is a bug in the DBMS core please provide simpler
steps to reproduce, ideally with pgsql utility and maybe bash.)
--
Best regards,
Aleksander Alekseev
ode is untouched.
The patch passed all the checks I could come up with.
--
Best regards,
Aleksander Alekseev
so we obtain lock on the current tuple version in each such relation before
executing the recheck.
"""
[1]:
https://github.com/postgres/postgres/blob/master/src/backend/executor/README#L381
--
Best regards,
Aleksander Alekseev
doesn't contradict the general guarantees
promised by READ COMMITTED.
Perhaps we should update the documentation for this case, or maybe
remove the quoted part of it.
Thoughts?
[1]: https://www.postgresql.org/docs/current/transaction-iso.html
--
Best regards,
Aleksander Alekseev
out the same thing?
It doesn't matter what will be used as a sign of presence of a varint
bitmask in the pointer. My initial proposal to use ToastCompressionId
for this is probably redundant since va_tag > 18 will already tell
that.
--
Best regards,
Aleksander Alekseev
articular statement is incorrect:
> This will allow us to extend the pointers indefinitely.
varatt_external is going to be limited to 255. But it seems to be a
reasonable limitation for the nearest 10-20 years or so.
[1]: https://commitfest.postgresql.org/39/3820/
--
Best regards,
Aleksander Alekseev
the simpler option
Agree. However, I think we will have to add the display of the port
number to "pg_ctl status" too, for the sake of consistency [1].
[1]: https://www.postgresql.org/docs/current/app-pg-ctl.html
--
Best regards,
Aleksander Alekseev
3ad0#l178
--
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
ou 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
me;
count
-
1048576
```
So I'm still unable to reproduce the described scenario, at least on PG16.
--
Best regards,
Aleksander Alekseev
. 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
e more beneficial for the community in
the long term.
Thoughts?
[1]: https://commitfest.postgresql.org/43/3626/
--
Best regards,
Aleksander Alekseev
iki/Mailing_Lists#Email_etiquette_mechanics
--
Best regards,
Aleksander Alekseev
gw_library_name
```
I'm pretty confident something went wrong with the parentheses in v2.
--
Best regards,
Aleksander Alekseev
*/
+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
y 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
ow(2048,3) > pow(2,32) - 1
True
```
Hopefully I didn't miss or misunderstood anything.
Thoughts?
--
Best regards,
Aleksander Alekseev
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
Or can'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
e 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
o address. I
don't think we should encourage the users to build unreliable systems.
--
Best regards,
Aleksander Alekseev
specifies 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
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
prepare
an updated patch for the next CF so I could use early feedback.
--
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
>
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
ect 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
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_use
(), 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.\n"
> + errhint("VA
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
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 it: It's slow,
> impossible to monitor, disable
rc/test/modules/test_resowner/sql/test_resowner.sql:
No such file
```
I wonder why the tests pass when using Meson.
--
Best regards,
Aleksander Alekseev
e are only a few days left if we are going to do this
within March CF...
--
Best regards,
Aleksander Alekseev
lvherre.pgsql
--
Best regards,
Aleksander Alekseev
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
his one is fine since store_conn_addrinfo() is going to fail
only when we are out of memory.
--
Best regards,
Aleksander Alekseev
; 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
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
ed to change the status of the patchset to RfC in a bit, unless
anyone has a second opinion.
--
Best regards,
Aleksander Alekseev
looks like the hint regarding replication slots was added at some
point. Currently we have:
```
errhint( [...]
"You might also need to commit or roll back old prepared
transactions, or drop stale replication slots.")));
```
So I choose to keep it as is for now. Please let me know if you
s happening in the "Compression dictionaries" thread
now, since we decided to join our efforts in this area, see the latest
messages. I suggest marking this thread as RwF, unless anyone objects.
[1]:
https://www.postgresql.org/message-id/CAH2-Wz%3D3mmHST-t9aR5LNkivXC%2B18JD_XC0ht4y5LQBLzq%2Bpsg%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
Hi,
> Again, MSDN claims to use a new API.
TWIMC the patch rotted a bit and now needs to be updated [1].
I changed its status to "Waiting on Author" [2].
[1]: http://cfbot.cputube.org/
[2]: https://commitfest.postgresql.org/42/3893/
--
Best regards,
Aleksander Alekseev
Hi,
> On Wed, Mar 8, 2023 at 7:30 PM Himanshu Upadhyaya
> wrote:
> Please find the v11 patch with all these changes.
The patch needed a rebase due to a4f23f9b. PFA v12.
--
Best regards,
Aleksander Alekseev
v12-0001-Implement-HOT-chain-validation-in-verify_heapam.patch
Descriptio
ward but I don't want to rush things and want to make sure
> I didn't miss something.
PFA the patch v57 which now includes both 1 and 2.
--
Best regards,
Aleksander Alekseev
v57-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
v57-0002-Use-larger
t max_prepared_transactions=0. So it seems
> to me (without testing) that this ought to be required. Did you test
> that it works without this setting?
Sorry, I was wrong the first time. The test fails without this line:
```
112/238 postgresql:pg_amcheck / pg_amcheck/004_verify_heapam ERROR
4.94s
.
```
+$node->append_conf('postgresql.conf','max_prepared_transactions=100');
```
>From what I can tell this line is not needed.
--
Best regards,
Aleksander Alekseev
C with the default value of
65536 as it is now. If the new GUC will receive a push back from the
community we can always use a hard-coded value instead, or no limit at
all.
[1]:
https://www.postgresql.org/docs/current/runtime-config-wal.html#GUC-WAL-BUFFERS
--
Best regards,
Aleksander Alekseev
v56-00
_root?
[1]: https://github.com/afiskon/pgscripts/blob/master/code-coverage.sh
[2]: https://www.postgresql.org/docs/current/xfunc-volatility.html
--
Best regards,
Aleksander Alekseev
tch
simplified.
--
Best regards,
Aleksander Alekseev
story, etc). I will
be happy to propose a follow-up patch accompanied by the benchmarks if
and when we reach the consensus on this patch.
--
Best regards,
Aleksander Alekseev
in this area is somewhat limited and I can't tell whether
this is OK. I will investigate but also I could use some feedback from
the reviewers.
--
Best regards,
Aleksander Alekseev
v55-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
nately I used up all the
mana for today and have to take a long rest in order to continue.
--
Best regards,
Aleksander Alekseev
v54-0001-Index-SLRUs-by-64-bit-integers-rather-than-by-32.patch
Description: Binary data
Hi Andres,
> Shouldn't need to extract the column if we just want to know if it's NULL (see
> heap_attisnull()). Afaics the value isn't accessed after this.
Many thanks. Fixed.
--
Best regards,
Aleksander Alekseev
v2-0001-Support-SK_SEARCHNULL-SK_SEARCHNOTNULL-for-heap-o.patch
Descr
workarounds come to mind this limitation may be
really of inconvenience for the extension authors, and implementing
corresponding support seems to be pretty straightforward.
The attached patch does this.
--
Best regards,
Aleksander Alekseev
v1-0001-Support-SK_SEARCHNULL-SK_SEARCHNOTNULL-for-heap
eparately (similarly to how TOAST does this)?
--
Best regards,
Aleksander Alekseev
m manually) and often have to process any
> garbage the application developer passes to an extension.
>
> This of course is applicable not only to extensions, but to any
> middleware between the DBMS and the application.
This however is arguably a niche use case. So maybe we don't want to
spend time on this.
--
Best regards,
Aleksander Alekseev
201 - 300 of 709 matches
Mail list logo
