Re: Proposal to Enable/Disable Index using ALTER INDEX

2024-09-23 Thread Maciek Sakrejda
If one of the use cases is soft-dropping indexes, would a GUC approach still support that? ALTER TABLE?

Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-09-09 Thread Maciek Sakrejda
So, there was limited enthusiasm for a new message here, but I noticed that current CF messages don't include the CF entry link [1]. What about adding a link to its existing e-mails? Thanks, Maciek [1]: e.g., https://www.postgresql.org/message-id/172579582925.1126.2395496302629349708.pgcf%40cori

Re: [PATCH] Add roman support for to_number function

2024-09-07 Thread Maciek Sakrejda
Sorry, it looks like I failed to accurately log my review in the review app due to the current broken layout issues [1]. The summary should be: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested (not sure what the spec has to say

Re: [PATCH] Add roman support for to_number function

2024-09-07 Thread Maciek Sakrejda
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, failed Spec compliant: tested, failed Documentation:tested, passed Tested again, and the patch looks good. It does not accept le

Re: [PATCH] Add roman support for to_number function

2024-09-03 Thread Maciek Sakrejda
On Tue, Sep 3, 2024 at 6:29 AM Hunaid Sohail wrote: > I submitted the patch on Aug 30 because I read that new patches should be > submitted in CF with "Open" status. Oh my bad! I missed that you had submitted it to the September CF: https://commitfest.postgresql.org/49/5221/ I don't see a way t

Re: [PATCH] Add roman support for to_number function

2024-09-02 Thread Maciek Sakrejda
Thanks for the contribution. I took a look at the patch, and it works as advertised. It's too late for the September commitfest, but I took the liberty of registering your patch for the November CF [1]. In the course of that, I found an older thread proposing this feature seven years ago [2]. That

Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

2024-08-16 Thread Maciek Sakrejda
On Wed, Aug 14, 2024 at 3:40 PM Jelte Fennema-Nio wrote: > > I'd like to send an automatic mail to a thread whenever it gets added > to a commitfest. Since this would impact everyone that's subscribed to > the mailinglist I'd like some feedback on this. This mail would > include: > > 1. A very sho

Re: Unused expression indexes

2024-08-07 Thread Maciek Sakrejda
Great, thank you both for the info.

Re: Unused expression indexes

2024-08-06 Thread Maciek Sakrejda
On Tue, Aug 6, 2024 at 1:25 PM Tom Lane wrote: > The planner has no info about the values of sqrt(x1), so you get a > default estimate (one-third) of the selectivity of the WHERE clause. > But watch this: > > regression=# create index on foo (sqrt(x1)); > CREATE INDEX > regression=# analyze foo; >

Unused expression indexes

2024-08-06 Thread Maciek Sakrejda
In a blog post [1], Bruce Momjian notes that expression indexes can help with planning even if they're not used directly. But the examples cited in that post are vague (i.e., they improve stats, but it's not clear how they could change plans), and Bruce's answer to a comment [2] suggests that this

Re: Casts from jsonb to other types should cope with json null

2024-08-01 Thread Maciek Sakrejda
On Thu, Aug 1, 2024 at 3:52 PM Tom Lane wrote: > I complained in the discussion of bug #18564 [1] that it's quite > inconsistent that you can cast a jsonb null to text and get > a SQL NULL: > > =# select ('{"a": null}'::jsonb)->>'a'; > ?column? > -- > > (1 row) Oddly, it looks like you o

Re: commitfest.postgresql.org is no longer fit for purpose

2024-05-20 Thread Maciek Sakrejda
On Sun, May 19, 2024 at 10:50 PM Thomas Munro wrote: > Sometimes I question the sanity of the whole thing. Considering > cfbot's original "zero-effort CI" goal (or I guess "zero-extra-effort" > would be better), I was curious about what other projects had the same > idea, or whether we're really

Re: commitfest.postgresql.org is no longer fit for purpose

2024-05-16 Thread Maciek Sakrejda
Thanks for raising this. As someone who is only modestly familiar with Postgres internals or even C, but would still like to contribute through review, I find the current process of finding a suitable patch both tedious and daunting. The Reviewing a Patch article on the wiki [0] says reviews like m

Re: Possibility to disable `ALTER SYSTEM`

2024-03-27 Thread Maciek Sakrejda
On Wed, Mar 27, 2024, 11:46 Robert Haas wrote: > On Wed, Mar 27, 2024 at 1:12 PM Isaac Morland > wrote: > > On Wed, 27 Mar 2024 at 13:05, Greg Sabino Mullane > wrote: > >>> The purpose of the setting is to prevent > accidental modifications via ALTER > SYSTEM in environments where > >> The emph

Re: Possibility to disable `ALTER SYSTEM`

2024-03-18 Thread Maciek Sakrejda
On Mon, Mar 18, 2024 at 10:27 AM Robert Haas wrote: > Right, we're adding this because of environments like Kubernetes, > which isn't a version, but it is a platform, or at least a deployment > mode, which is why I thought of that section. I think for now we > should just file this under "Other pl

Re: Possibility to disable `ALTER SYSTEM`

2024-03-18 Thread Maciek Sakrejda
On Mon, Mar 18, 2024 at 7:12 AM Jelte Fennema-Nio wrote: > > On Mon, 18 Mar 2024 at 13:57, Robert Haas wrote: > > I would have been somewhat inclined to find an existing section > > of postgresql.auto.conf for this parameter, perhaps "platform and > > version compatibility". > > I tried to find a

Re: Possibility to disable `ALTER SYSTEM`

2024-03-14 Thread Maciek Sakrejda
On Thu, Mar 14, 2024 at 1:38 PM Robert Haas wrote: > On Thu, Mar 14, 2024 at 4:08 PM Tom Lane wrote: > > The patch-of-record contains no such wording. > > I plan to fix that, if nobody else beats me to it. > > > And if this isn't a > > security feature, then what is it? If you have to say to you

Re: Printing backtrace of postgres processes

2024-01-14 Thread Maciek Sakrejda
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed I'm not sure if this actually still needs review, but it's marked

Re: Pdadmin open on Macbook issue

2023-12-28 Thread Maciek Sakrejda
This is not the right mailing list for your question. Try the pgadmin-support [1] mailing list. You may also want to include more details in your question, because it's not really possible to tell what's going wrong from your description. [1]: https://www.postgresql.org/list/pgadmin-support/

Re: Proposal: In-flight explain logging

2023-12-02 Thread Maciek Sakrejda
Have you seen the other recent patch regarding this? [1] The mailing list thread was active pretty recently. The submission is marked as Needs Review. I haven't looked at either patch, but the proposals are very similar as I understand it. [1]: https://commitfest.postgresql.org/45/4345/

Re: Emitting JSON to file using COPY TO

2023-12-02 Thread Maciek Sakrejda
On Fri, Dec 1, 2023 at 11:32 AM Joe Conway wrote: > 1. Is supporting JSON array format sufficient, or does it need to > support some other options? How flexible does the support scheme need to be? "JSON Lines" is a semi-standard format [1] that's basically just newline-separated JSON values. (In

Re: run pgindent on a regular basis / scripted manner

2023-10-17 Thread Maciek Sakrejda
On Tue, Oct 17, 2023, 09:22 Robert Haas wrote: > On Tue, Oct 17, 2023 at 12:18 PM Tom Lane wrote: > > Robert Haas writes: > > Is that actually possible? I had the idea that "git push" is an > > atomic operation, ie 100% or nothing. Is it only atomic per-branch? > > I believe so. Git push do

Re: Differences between = ANY and IN?

2023-10-02 Thread Maciek Sakrejda
Great, thanks for the guidance!

Re: pg_stat_statements and "IN" conditions

2023-10-02 Thread Maciek Sakrejda
I've also tried the patch and I see the same results as Jakub, which make sense to me. I did have issues getting it to apply, though: `git am` complains about a conflict, though patch itself was able to apply it.

Differences between = ANY and IN?

2023-10-02 Thread Maciek Sakrejda
Hello, My colleague has been working on submitting a patch [1] to the Ruby Rails framework to address some of the problems discussed in [2]. Regardless of whether that change lands, the change in Rails would be useful since people will be running Postgres versions without this patch for a while.

Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }

2023-09-19 Thread Maciek Sakrejda
On Tue, Sep 19, 2023, 20:23 Maciek Sakrejda wrote: > I wonder if something like CURRENT (i.e., the search path at function > creation time) might be a useful keyword addition. I can see some uses > (more forgiving than SYSTEM but not as loose as SESSION), but I don't > know if

Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }

2023-09-19 Thread Maciek Sakrejda
On Tue, Sep 19, 2023 at 5:56 PM Jeff Davis wrote: >... > On Tue, 2023-09-19 at 11:41 -0400, Robert Haas wrote: > > That leads to a second idea, which is having it continue > > to be a GUC but only affect directly-entered SQL, with all > > indirectly-entered SQL either being stored as a node tree o

Re: Overhauling "Routine Vacuuming" docs, particularly its handling of freezing

2023-05-01 Thread Maciek Sakrejda
On Mon, May 1, 2023, 18:08 Robert Haas wrote: > I am saying that, while wraparound is perhaps not a perfect term > for what's happening, it is not, in my opinion, a bad term either. I don't want to put words into Peter's mouth, but I think that he's arguing that the term "wraparound" suggests t

Re: doc: add missing "id" attributes to extension packaging page

2023-04-05 Thread Maciek Sakrejda
For what it's worth, I think having the anchors be always-visible when CSS disabled is a feature. The content is still perfectly readable, and the core feature from this patch is available. Introducing JavaScript to lose that functionality seems like a step backwards. By the way, the latest patch

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

2023-02-23 Thread Maciek Sakrejda
On Thu, Feb 23, 2023, 09:55 Nikolay Samokhvalov wrote: > On Thu, Feb 23, 2023 at 9:05 AM Maciek Sakrejda > wrote: > >> I think Heikki's solution is probably more practical since (1) .. > > > Note that these ideas target two *different* problems: > - what wa

Re: Proposal: %T Prompt parameter for psql for current time (like Oracle has)

2023-02-23 Thread Maciek Sakrejda
+1 on solving the general problem of "I forgot to set \timing--how long did this run?". I could have used that more than once in the past, and I'm sure it will come up again. I think Heikki's solution is probably more practical since (1) even if we add the prompt parameter originally proposed, I d

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2023-02-14 Thread Maciek Sakrejda
On Tue, Feb 14, 2023 at 11:08 AM Andres Freund wrote: > One thing I started to wonder about since is whether we should remove the io_ > prefix from io_object, io_context. The prefixes make sense on the C level, but > it's not clear to me that that's also the case on the table level. Yeah, +1. It'

Re: ANY_VALUE aggregate

2023-02-14 Thread Maciek Sakrejda
I could have used such an aggregate in the past, so +1. This is maybe getting into nit-picking, but perhaps it should be documented as returning an "arbitrary" value instead of a "non-deterministic" one? Technically the value is deterministic: there's a concrete algorithm specifying how it's selec

Re: Something is wrong with wal_compression

2023-01-27 Thread Maciek Sakrejda
On Fri, Jan 27, 2023, 18:58 Andres Freund wrote: > Hi, > > On 2023-01-27 16:15:08 +1300, Thomas Munro wrote: > > It would be pg_current_xact_id() that would have to pay the cost of > > the WAL flush, not pg_xact_status() itself, but yeah that's what the > > patch does (with some optimisations).

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2023-01-17 Thread Maciek Sakrejda
On Tue, Jan 17, 2023 at 9:22 AM Melanie Plageman wrote: > On Mon, Jan 16, 2023 at 4:42 PM Maciek Sakrejda wrote: > > I missed a couple of versions, but I think the docs are clearer now. > > I'm torn on losing some of the detail, but overall I do think it's a > &

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2023-01-16 Thread Maciek Sakrejda
On Fri, Jan 13, 2023 at 10:38 AM Melanie Plageman wrote: > > Attached is v47. I missed a couple of versions, but I think the docs are clearer now. I'm torn on losing some of the detail, but overall I do think it's a good trade-off. Moving some details out to after the table does keep the bulk of

Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

2023-01-02 Thread Maciek Sakrejda
On Fri, Jul 15, 2022 at 11:21 AM Maciek Sakrejda wrote: > On Fri, Jul 1, 2022 at 10:26 AM Andres Freund wrote: > > On 2022-07-01 01:23:01 -0700, Lukas Fittl wrote: > >... > > > Known WIP problems with this patch version: > > > > > > * There appears to be

Re: GetNewObjectId question

2022-12-10 Thread Maciek Sakrejda
Sure. My C is pretty limited, but I think it's just the attached? I patterned the usage on the way this is done in CreateRole. It passes check-world here. From c7cae5d3e8d179505f26851f1241436a3748f9a8 Mon Sep 17 00:00:00 2001 From: Maciek Sakrejda Date: Sat, 10 Dec 2022 22:51:02 -0800 Su

GetNewObjectId question

2022-12-10 Thread Maciek Sakrejda
While browsing through varsup.c, I noticed this comment on GetNewObjectId: * Hence, this routine should generally not be used directly. The only direct * callers should be GetNewOidWithIndex() and GetNewRelFileNumber() in * catalog/catalog.c. But AddRoleMems in user.c appears to also call the fu

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-12-07 Thread Maciek Sakrejda
In the pg_stat_statements docs, there are several column descriptions like Total number of ... by the statement You added an additional sentence to some describing the equivalent pg_stat_io values, but you only added a period to the previous sentence for shared_blks_read (for other columns, th

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-12-04 Thread Maciek Sakrejda
t that time had you leveraged the freelist. I think the > decision about which tradeoff to make is quite contentious, though. Thanks for the explanation--that makes sense. > On Mon, Nov 7, 2022 at 1:26 PM Maciek Sakrejda wrote: > > Alternately, what do you think about pulling equivalencies to

Odd behavior with pg_stat_statements and queries called from SQL functions

2022-11-16 Thread Maciek Sakrejda
I noticed an odd behavior today in pg_stat_statements query normalization for queries called from SQL-language functions. If I have three functions that call an essentially identical query (the functions are only marked SECURITY DEFINER to prevent inlining): maciek=# create or replace function f1(

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-11-07 Thread Maciek Sakrejda
On Thu, Nov 3, 2022 at 10:00 AM Melanie Plageman wrote: > > I decided not to call it pg_statio because all of the other stats views > have an underscore after stat and I thought it was an opportunity to be > consistent with them. Oh, got it. Makes sense. > > I'm reviewing the rendered docs now,

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-10-30 Thread Maciek Sakrejda
On Wed, Oct 26, 2022 at 10:55 AM Melanie Plageman wrote: + The pg_statio_ and + pg_stat_io views are primarily useful to determine + the effectiveness of the buffer cache. When the number of actual disk reads Totally nitpicking, but this reads a little funny to me. Previously the trailing unders

Re: warn if GUC set to an invalid shared library

2022-10-30 Thread Maciek Sakrejda
On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby wrote: > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > It caused no issue when I changed: > > > > /* Check that it's acceptable for the indicated > > parameter */ > > if (!parse_a

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-10-23 Thread Maciek Sakrejda
On Thu, Oct 20, 2022 at 10:31 AM Andres Freund wrote: > - "repossession" is a very unintuitive name for me. If we want something like > it, can't we just name it reuse_failed or such? +1, I think "repossessed" is awkward. I think "reuse_failed" works, but no strong opinions on an alternate name

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-10-23 Thread Maciek Sakrejda
On Wed, Oct 19, 2022 at 12:27 PM Melanie Plageman wrote: > > v34 is attached. > I think the column names need discussion. Also, the docs need more work > (I added a lot of new content there). I could use feedback on the column > names and definitions and review/rephrasing ideas for the docs > addi

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-10-16 Thread Maciek Sakrejda
On Thu, Oct 13, 2022 at 10:29 AM Melanie Plageman wrote: > I think that it makes sense to count both the initial buffers added to > the ring and subsequent shared buffers added to the ring (either when > the current strategy buffer is pinned or in use or when a bulkread > rejects dirty strategy bu

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

2022-10-10 Thread Maciek Sakrejda
Thanks for working on this! Like Lukas, I'm excited to see more visibility into important parts of the system like this. On Mon, Oct 10, 2022 at 11:49 AM Melanie Plageman wrote: > > I've gone ahead and implemented option 1 (commented below). No strong opinion on 1 versus 2, but I guess at least

Re: warn if GUC set to an invalid shared library

2022-07-22 Thread Maciek Sakrejda
Thanks for picking this back up, Justin. >I've started to think that we should really WARN whenever a (set of) GUC is set >in a manner that the server will fail to start - not just for shared libraries. +0.5. I think it's a reasonable change, but I've never broken my server with anything other th

Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?

2022-07-15 Thread Maciek Sakrejda
I ran that original test case with and without the patch. Here are the numbers I'm seeing: master (best of three): postgres=# SELECT count(*) FROM lotsarows; Time: 582.423 ms postgres=# EXPLAIN (ANALYZE, TIMING OFF) SELECT count(*) FROM lotsarows; Time: 616.102 ms postgres=# EXPLAIN (ANALYZE, T

Re: Add id's to various elements in protocol.sgml

2022-02-24 Thread Maciek Sakrejda
On Thu, Feb 24, 2022, 16:52 Kyotaro Horiguchi wrote: > At Tue, 21 Dec 2021 08:47:27 +0100, Brar Piening wrote in > > On 20.12.2021 at 16:09, Robert Haas wrote: > > > As a data point, this is something I have also wanted to do, from time > > > to time. I am generally of the opinion that any place

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

2022-02-14 Thread Maciek Sakrejda
Andrew made a good case above for avoiding LOG: >I do think we should be wary of any name starting with "LOG", though. >Long experience tells us that's something that confuses users when it refers to the WAL.

Re: warn if GUC set to an invalid shared library

2022-02-14 Thread Maciek Sakrejda
On Wed, Feb 9, 2022 at 5:58 PM Justin Pryzby wrote: > FYI, it has said "while..." and hasn't said "guc" since the 2nd revision of > the > patch. The v3-0001 attached above had "while... for GUC..."--sorry I wasn't clear. In v4, the message looks fine to me for shared_preload_libraries (except t

Re: CREATEROLE and role ownership hierarchies

2022-02-03 Thread Maciek Sakrejda
I'm chiming in a little late here, but as someone who worked on a system to basically work around the lack of unprivileged CREATE ROLE for a cloud provider (I worked on the Heroku Data team for several years), I thought it might be useful to offer my perspective. This is, of course, not the only us

Re: warn if GUC set to an invalid shared library

2022-02-03 Thread Maciek Sakrejda
On Wed, Feb 2, 2022 at 7:39 AM David G. Johnston wrote: > I would at least consider having the UX go something like: > > postgres=# ALTER SYSTEM SET shared_preload_libraries = not_such_library; > ERROR: that library is not present>. > HINT: to bypass the error please add FORCE before SET > postg

Re: warn if GUC set to an invalid shared library

2022-02-01 Thread Maciek Sakrejda
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:not tested I tried the latest version of the patch, and it works as discussed. T

Re: warn if GUC set to an invalid shared library

2022-01-09 Thread Maciek Sakrejda
On Sat, Jan 8, 2022 at 2:07 PM Justin Pryzby wrote: > Unfortunately, the output for dlopen() is not portable, which (I think) means > most of what I wrote can't be made to work.. Since it doesn't work to call > dlopen() when using SET, I tried using just stat(). But that also fails on > windows,

Re: warn if GUC set to an invalid shared library

2022-01-08 Thread Maciek Sakrejda
On Thu, Dec 30, 2021 at 12:21 AM Bharath Rupireddy wrote: > Overall the idea looks good to me. A warning on ALTER SYSTEM SET seems > reasonable than nothing. ERROR isn't the way to go as it limits the > users of setting the extensions in shared_preload_libraries first, > installing them later. Is

Re: warn if GUC set to an invalid shared library

2022-01-08 Thread Maciek Sakrejda
Thanks for working on this! I tried it out and it worked for me. I reviewed the patch and didn't see any problems, but I'm not much of a C programmer. On Tue, Dec 28, 2021 at 9:45 AM Justin Pryzby wrote: > 0002 adds context when failing to start. > > 2021-12-27 17:01:12.996 CST postmaster

Re: [EXTERNAL] Re: should we document an example to set multiple libraries in shared_preload_libraries?

2021-12-18 Thread Maciek Sakrejda
On Fri, Dec 10, 2021 at 10:10 AM Godfrin, Philippe E wrote: > I may have missed something in this stream, but is this a system controlled > by Patroni? In my case, no: it's a pretty vanilla PGDG install on Ubuntu 20.04. Thanks for the context, though. Thanks, Maciek

Re: should we document an example to set multiple libraries in shared_preload_libraries?

2021-12-18 Thread Maciek Sakrejda
On Thu, Dec 9, 2021 at 7:42 AM Bharath Rupireddy wrote: > How about ALTER SYSTEM SET shared_preload_libraries command itself > checking for availability of the specified libraries (after fetching > library names from the parsed string value) with stat() and then > report an error if any of the lib

Re: should we document an example to set multiple libraries in shared_preload_libraries?

2021-12-08 Thread Maciek Sakrejda
On Wed, Dec 1, 2021 at 5:15 AM Tom Lane wrote: > > Justin Pryzby writes: > > +1 to document it, but it seems like the worse problem is allowing the > > admin to > > write a configuration which causes the server to fail to start, without > > having > > issued a warning. > > > I think you could f

TOAST questions

2021-07-07 Thread Maciek Sakrejda
Hello, (I hope it's okay to ask general internals questions here; if this list is strictly for development, I'll keep my questions on -general but since I'm asking about internal behavior, this seemed more appropriate.) I was playing around with inspecting TOAST tables in order to understand the

Re: compute_query_id and pg_stat_statements

2021-05-13 Thread Maciek Sakrejda
On Thu, May 13, 2021 at 8:38 AM Julien Rouhaud wrote: > > On Thu, May 13, 2021 at 08:32:50AM -0700, Maciek Sakrejda wrote: > > > > The warning makes it clear that there's something wrong, but not how > > to fix it > > I'm confused, are we talking abo

Re: compute_query_id and pg_stat_statements

2021-05-13 Thread Maciek Sakrejda
On Thu, May 13, 2021 at 7:42 AM Bruce Momjian wrote: > > On Thu, May 13, 2021 at 12:03:42PM +0800, Julien Rouhaud wrote: > > On Wed, May 12, 2021 at 11:33:32PM -0400, Bruce Momjian wrote: > > I don't know what to say. So here is a summary of the complaints that I'm > > aware of: > > > > - > > ht

Re: compute_query_id and pg_stat_statements

2021-05-12 Thread Maciek Sakrejda
On Wed, May 12, 2021 at 9:58 PM Julien Rouhaud wrote: > Le jeu. 13 mai 2021 à 12:52, Maciek Sakrejda a écrit : >> >> For what it's worth, I don't think the actual changing of an extra >> setting is that big a burden: it's the figuring out that you need

Re: compute_query_id and pg_stat_statements

2021-05-12 Thread Maciek Sakrejda
On Wed, May 12, 2021 at 9:03 PM Julien Rouhaud wrote: > > On Wed, May 12, 2021 at 11:33:32PM -0400, Bruce Momjian wrote: > > How do they know to set shared_preload_libraries then? We change the > > user API all the time, especially in GUCs, and even rename them, but for > > some reason we don't t

Re: pg_stat_statements requires compute_query_id

2021-05-10 Thread Maciek Sakrejda
On Mon, May 10, 2021 at 7:43 AM Julien Rouhaud wrote: > On Mon, May 10, 2021 at 04:36:16PM +0200, Pavel Stehule wrote: > > I expected just an empty column query_id and workable extension. This > > doesn't look well. > > > > More, it increases the (little bit) complexity of migration to Postgres 14

Re: EXPLAIN: Non-parallel ancestor plan nodes exclude parallel worker instrumentation

2020-06-25 Thread Maciek Sakrejda
ings are rolled up into the parallel leader, and that is propagated as expected to the Gather. Sorry for the confusion. On Wed, Jun 24, 2020 at 3:18 AM Maciek Sakrejda wrote: >So we should be seeing an average, not a sum, right? And here I missed that the documentation specifies rows and actual

Re: EXPLAIN: Non-parallel ancestor plan nodes exclude parallel worker instrumentation

2020-06-24 Thread Maciek Sakrejda
On Tue, Jun 23, 2020 at 7:55 PM Amit Kapila wrote: > > I don't see any other reason for > > looping over the NL node itself in this plan. The Gather itself > > doesn't do any real looping, right? > > It is right that Gather doesn't do looping but Parallel Seq Scan node does so. Sorry, I still don

Re: EXPLAIN: Non-parallel ancestor plan nodes exclude parallel worker instrumentation

2020-06-23 Thread Maciek Sakrejda
On Tue, Jun 23, 2020 at 2:57 AM Amit Kapila wrote: > I don't think this is an odd situation because in this case, child > nodes like "Nested Loop" and "Parallel Seq Scan" has a value of > 'loops' as 3. So, to get the correct stats at those nodes, you need > to divide it by 3 whereas, at Gather no

EXPLAIN: Non-parallel ancestor plan nodes exclude parallel worker instrumentation

2020-06-22 Thread Maciek Sakrejda
Hello, I had some questions about the behavior of some accounting in parallel EXPLAIN plans. Take the following plan: ``` Gather (cost=1000.43..750173.74 rows=2 width=235) (actual time=1665.122..1665.122 rows=0 loops=1) Workers Planned: 2 Workers Launched: 2 Buffers: shared hit=27683 read=

Re: JIT performance bug/regression & JIT EXPLAIN

2020-01-27 Thread Maciek Sakrejda
On Mon, Jan 27, 2020 at 11:01 AM Robert Haas wrote: > On Fri, Nov 15, 2019 at 8:05 PM Maciek Sakrejda wrote: > > On Fri, Nov 15, 2019 at 5:49 AM Robert Haas wrote: > > > Personally, I don't care very much about backward-compatibility, or > > > about how hard it

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-24 Thread Maciek Sakrejda
Thanks for the thorough review. I obviously missed some critical issues. I recognized some of the other maintainability problems, but did not have a sense of how to best avoid them, being unfamiliar with the code. For what it's worth, this version makes sense to me.

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-24 Thread Maciek Sakrejda
Okay. Does not getting as many workers as it asks for include sometimes getting zero, completely changing the actual output? If so, I'll submit a new version of the patch removing all tests--I was hoping to improve coverage, but I guess this is not the way to start. If not, can we keep the json tes

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-24 Thread Maciek Sakrejda
Great, thank you. I noticed in the CF patch tester app, the build fails on Windows [1]. Investigating, I realized I had failed to fully strip volatile EXPLAIN info (namely buffers) in TEXT mode due to a bad regexp_replace. I've fixed this in the attached patch (which I also rebased against current

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-23 Thread Maciek Sakrejda
On Wed, Jan 22, 2020 at 9:37 AM Georgios Kokolatos wrote: > My bad, I should have been more clear. I meant that it is preferable to use > the C99 standard which calls for declaring variables in the scope that you > need them. Ah, I see. I think I got that from ExplainPrintSettings. I've corrected

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-22 Thread Maciek Sakrejda
Thanks! I'll fix the brace issues. Re: the other items: > + int num_workers = planstate->worker_instrument->num_workers; > + int n; > + worker_strs = (StringInfo *) palloc0(num_workers * > sizeof(StringInfo)); > + for (n = 0; n < num_workers; n++) { > > I think C99 would b

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-21 Thread Maciek Sakrejda
TEXT format was tricky due to its inconsistencies, but I think I have something working reasonably well. I added a simple test for TEXT format output as well, using a similar approach as the JSON format test, and liberally regexp_replacing away any volatile output. I suppose in theory we could do t

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-15 Thread Maciek Sakrejda
Sounds good, I'll try that format. Any idea how to test YAML? With the JSON format, I was able to rely on Postgres' own JSON-manipulating functions to strip or canonicalize fields that can vary across executions--I can't really do that with YAML. Or should I run EXPLAIN with COSTS OFF, TIMING OFF,

Re: Duplicate Workers entries in some EXPLAIN plans

2020-01-14 Thread Maciek Sakrejda
Thanks for the review! I looked at and rebased the patch on current master, ac5bdf6. I introduced a new test file because this bug is specifically about EXPLAIN output (as opposed to query execution or planning functionality), and it didn't seem like a test would fit in any of the other files. I

Re: Duplicate Workers entries in some EXPLAIN plans

2019-12-27 Thread Maciek Sakrejda
Done! Thanks!

Re: Duplicate Workers entries in some EXPLAIN plans

2019-12-26 Thread Maciek Sakrejda
I wanted to follow up on this patch since I received no feedback. What should my next steps be (besides rebasing, though I want to confirm there's interest before I do that)?

Re: Duplicate Workers entries in some EXPLAIN plans

2019-11-18 Thread Maciek Sakrejda
On Thu, Oct 24, 2019 at 6:48 PM Andres Freund wrote: > Unfortunately I think the fix isn't all that trivial, due to the way we > output the per-worker information at the end of ExplainNode(), by just > dumping things into a string. It seems to me that a step in the right > direction would be for

Re: JIT performance bug/regression & JIT EXPLAIN

2019-11-15 Thread Maciek Sakrejda
On Fri, Nov 15, 2019 at 5:49 AM Robert Haas wrote: > Personally, I don't care very much about backward-compatibility, or > about how hard it is for tools to parse. I want it to be possible, but > if it takes a little extra effort, so be it. I think these are two separate issues. I agree on backwa

Re: JIT performance bug/regression & JIT EXPLAIN

2019-11-12 Thread Maciek Sakrejda
On Mon, Oct 28, 2019 at 5:02 PM Andres Freund wrote: > What I dislike about that is that it basically again is introducing "again"? Am I missing some history here? I'd love to read up on this if there are mistakes to learn from. > something that requires either pattern matching on key names (i.e

Re: JIT performance bug/regression & JIT EXPLAIN

2019-10-28 Thread Maciek Sakrejda
>But that's pretty crappy, because it means that the 'shape' of the output >depends on the jit_details option. Yeah, that would be hard to work with. What about adding it as a sibling group? "Filter": "(lineitem.l_shipdate <= '1998-09-18 00:00:00'::timestamp without time zone)", "Filter JIT": {

Duplicate Workers entries in some EXPLAIN plans

2019-10-22 Thread Maciek Sakrejda
Hello, I originally reported this in pgsql-bugs [0], but there wasn't much feedback there, so moving the discussion here. When using JSON, YAML, or XML-format EXPLAIN on a plan that uses a parallelized sort, the Sort nodes list two different entries for "Workers", one for the sort-related info, an