On Fri, Apr 25, 2025 at 4:13 PM David E. Wheeler wrote:
>
> On Apr 25, 2025, at 09:25, Matheus Alcantara wrote:
>
> > Yes, you are right. The problem was where I was asserting
> > control->control_dir != NULL. I've moved the assert after the "if
> > (!f
On Thu, Apr 24, 2025 at 7:27 PM David E. Wheeler wrote:
>
> On Apr 24, 2025, at 11:18, Matheus Alcantara wrote:
>
> > In v2 I've moved the logic to remove the /extension to
> > parse_extension_control_file(), do you think that this Assert on this
> > function woul
On Thu, Apr 24, 2025 at 7:21 AM Christoph Berg wrote:
>
> Re: Matheus Alcantara
> > I've tested with the semver extension and it seems to work fine with
> > this patch. Can you please check on your side to see if it's also
> > working?
>
> Hi Matheus,
>
with the semver extension and it seems to work fine with
this patch. Can you please check on your side to see if it's also
working?
I still want to make some polish on this patch and also include some
more test cases using the "directory" on the .control file but I think
that is stabl
privately IIUC we concluded that we may document
this limitation that using extension control path with an extension that
uses a custom "directory" field on .control file will not work. I think
that we may add a note section on "extension_control_path" doc on [2],
what do you think?
[1]
https://www.postgresql.org/message-id/0F50D989-B82D-4F59-9F13-C08A4673322C%40justatheory.com
[2]
https://www.postgresql.org/docs/devel/runtime-config-client.html#GUC-EXTENSION-CONTROL-PATH
--
Matheus Alcantara
.
--
Matheus Alcantara
hat guarantees
> that that's so, though. It'd fail if contrib hasn't been
> installed. Is there a reason to use "amcheck" rather than
> something more certainly available, like "plpgsql"?
There is no specific reason to use "amcheck" instea
_security_check(conn, rconn, connstr);
>
> - These comment additions probably belong in 0001 rather than 0002.
Fixed
> - As discussed offlist, 0002 needs pgperltidy'd rather than perltidy'd.
Fixed
> - I have attached some additional nitpicky comment edits and
> whitespace
that it would be good to move this part of the documentation to
0004 instead of 0007, what do you think?
--
Matheus Alcantara
On Fri, Mar 21, 2025 at 1:28 PM Jacob Champion
wrote:
>
> Great, thank you! Looking over v10, I think I've run out of feedback
> at this point. Marked Ready for Committer.
Thanks for all the effort reviewing this patch!
--
Matheus Alcantara
On Thu, Mar 27, 2025 at 4:42 PM Melanie Plageman
wrote:
>
> On Thu, Mar 27, 2025 at 2:46 PM Matheus Alcantara
> wrote:
> >
> > Just my 0.2 cents. I also like the first approach even though I prefer
> > the v4 version, but anyway, thanks very much for reviewing and
>
c and I marked the commitfest entry as such.
Just my 0.2 cents. I also like the first approach even though I prefer
the v4 version, but anyway, thanks very much for reviewing and
committing!
(I was sending my anwer just when I received your reply)
--
Matheus Alcantara
On Wed, Mar 26, 2025 at 7:41 AM Peter Eisentraut wrote:
>
> On 24.03.25 21:33, Matheus Alcantara wrote:
> >> I'm a bit confused about the refactoring patch 0001. There are some
> >> details there that don't seem right. For example, you write that the
>
On Mon, Mar 24, 2025 at 1:16 PM Peter Eisentraut wrote:
>
> On 21.03.25 19:24, Matheus Alcantara wrote:
> > On Fri, Mar 21, 2025 at 1:28 PM Jacob Champion
> > wrote:
> >>
> >> Great, thank you! Looking over v10, I think I've run out of feedback
>
hing bites us, and we're looking in the wrong place)
>
> Nope, testing shows it's not that, so I am rather confused about what was
> going on.
>
I'm not sure if I'm checking on the right place [1] but it seems that the
Contrib and ContribInstall is executed after Check step which causes this test
failure?
'steps_completed' => [
'SCM-checkout',
'Configure',
'Build',
'Check',
'Contrib',
'TestModules',
'Install',
'ContribInstall',
'TestModulesInstall',
'MiscCheck',
...
]
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snakefly&dt=2025-03-20%2009%3A46%3A05
--
Matheus Alcantara
d on get_extension_script_directory.
> 2. If the directory is not a full path, check for it under each
> directory in `extension_control_path`? But no, that points to
> `share/extension`, not `share`, so it can’t really searched unless it
> also lops off `extension` from the end of each path.
Maybe we could make the "extension" part of the extension control path
explicitly, like Peter has mentioned in his first patch version [1]?.
If "directory" is not set we could use "extension" otherwise use the
"directory" as a path suffix when searching on extension_control_path?
[1]
https://www.postgresql.org/message-id/0d384836-7e6e-4932-af3b-8dad1f6fee43%40eisentraut.org
--
Matheus Alcantara
On Thu, Mar 20, 2025 at 9:02 PM Jacob Champion
wrote:
>
> On Thu, Mar 20, 2025 at 12:54 PM Matheus Alcantara
> wrote:
> > Since the security checks are defined I'm attaching 0003 which include
> > the fix of security checks for postgres_fdw. It implements the
> >
[11:29:42.995](0.000s) # got: '1'
> > # expected: '0'
> > [11:29:42.995](0.000s) ok 2 - no parameters missing from guc_tables.c
> > [11:29:42.995](0.000s) ok 3 - no parameters marked as NOT_IN_SAMPLE in
> > postgresql.conf.sample
> > found GUC jit_cached in guc_tables.c, missing from
> > postgresql.conf.sample
>
> I tested in with make check and make installcheck . In v8 I found bugs,
> but not published fix yet.
>
Do you have any intent to work on a new version for this?
--
Matheus Alcantara
e more recent test changes.
> But I'm rapidly running out of feedback, so I think this is very
> close.
>
Fixed
Thanks very much for all the reviews on this patch!
--
Matheus Alcantara
v8-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
v8-0002-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
On Mon, Mar 17, 2025 at 1:49 PM Jacob Champion
wrote:
>
> On Thu, Mar 13, 2025 at 2:38 PM Matheus Alcantara
> wrote:
> > I thought about this; The problem is that at this point, the scram keys
> > on connection options are base64 encoded (see appendSCRAMKeysInfo), so
>
Hi,
On Sun, Mar 16, 2025 at 10:30 AM vignesh C wrote:
>
> On Wed, 12 Feb 2025 at 00:11, Matheus Alcantara
> wrote:
> >
> > Hi,
> >
> > Em ter., 11 de fev. de 2025 às 03:39, jian he
> > escreveu:
> > > hi. some minor issue i found.
> > &g
On Thu, Mar 13, 2025 at 4:54 PM Jacob Champion
wrote:
>
> On Thu, Mar 13, 2025 at 6:59 AM Matheus Alcantara
> wrote:
> > Fixed by wrapping on PG_CATCH/pfree/PG_RE_THROW. I didn't managed to
> > create a test that use this code path, so let me know if I'm still
&g
also changed the "connstr check" and
"security check" to have a validation very similar to what Peter
implemented on 0001-WIP-postgres_fdw-Fix-SCRAM-pass-through-security
patch. I also reproduced this test case that you've created on this new
dblink patch version and we actually
ot call
find_extension_control_filename instead of putting this logic there
since we already set the control_dir when we find the control file, and
having the logic to set the control_dir or skip the find_in_path seems
more confusing on this function instead of on
parse_extension_control_file. Please let me know what you think.
--
Matheus Alcantara
v7-0001-extension_control_path.patch
Description: Binary data
to export too many things from there.)
>
I've exported substitute_path_macro because adding a new function on
dfmgr would require #include nodes/pg_list.h and I'm not sure what
approach would be better, please let me know what you think.
--
Matheus Alcantara
v6-0001-extension_control_path.patch
Description: Binary data
3_check_guc.pl line 85.
[11:29:42.995](0.000s) # got: '1'
# expected: '0'
[11:29:42.995](0.000s) ok 2 - no parameters missing from guc_tables.c
[11:29:42.995](0.000s) ok 3 - no parameters marked as NOT_IN_SAMPLE in
postgresql.conf.sample
found GUC jit_cached in guc_tables.c, missing from postgresql.conf.sample
--
Matheus Alcantara
> Just bikeshedding a bit ...
>
> I'm not mad keen on this design. I think the value should be either a
> single setting like "WARNING" or a set of type:setting pairs. I agree
> that "all" is a bad name, but I think "default" would make sense.
>
> I can live with it but I think this just looks a bit odd.
>
Just bringing some thoughts about it...
How about using something like *:WARNING? I'm not sure if it could also be
confusing as the "all" keyword, but I think it could also be interpreted as
"anything else use WARNING".
--
Matheus Alcantara
On Mon, Mar 3, 2025 at 4:16 PM Matheus Alcantara
wrote:
>
> Hi Alex,
>
> The code comments and the commit messages help a lot when reviewing! Thanks
> for
> the new version.
>
> The code LGTM and check-world is happy. I've also performed some tests and
> ever
th "The make_jsonpath_item_* functions" since we can have more
functions in the future that are not fully related with these. Does that make
sense?
--
Matheus Alcantara
--
Matheus Alcantara
v5-0001-extension_control_path.patch
Description: Binary data
cify the extension name.
This is a similar behaviour when, e.g we have a where clause that is
referencing a column that is present in multiple tables used in the query:
ERROR: 42702: column reference "b" is ambiguous LINE 1: select * from t inner
join t2 on t.a = t2.a where b = 10;
--
Matheus Alcantara
Hi
On Tue, Feb 25, 2025 at 5:29 PM Matheus Alcantara
wrote:
> Fixed on the attached v3.
>
After I've sent the v3 patch I noticed that the tests were failing on windows.
The problem was on TAP test that was using ":" as a separator on
extension_control_path and also the path
trlen(Extension_control_path) == 0)
> +{
> +paths = lappend(paths, ecp);
> + }
>
Fixed on the attached v3.
--
Matheus Alcantara
v3-0001-extension_control_path.patch
Description: Binary data
Thanks for all the comments on this folks! I probably underestimated
this change.
Thanks all.
--
Matheus Alcantara
lp with this patch I'm attaching a new version with the remaining TODOs
fixed and also with a new TAP test.
Thoughts?
--
Matheus Alcantara
v2-0001-extension_control_path.patch
Description: Binary data
tring type only
on these specific commands.
Thoughts?
--
Matheus Alcantara
v1-0001-Redact-user-password-on-pg_stat_statements.patch
Description: Binary data
ON_OK)
>
> I don't think this should be changed in a refactoring patch.
> PQstatus() can handle a NULL conn pointer.
>
Fixed
> > - if (rconn)
> > - pfree(rconn);
>
> Looks like this code in dblink_connect() was dropped.
>
Oops, fixed on connect_pg_server since this logic was moved to this function.
## Some other changes
I also added a new TAP test case to ensure that we return an error if
require_auth is overwritten with another value.
## Questions:
- The new dblink_connstr_has_scam_require_auth function is very similar with
dblink_connstr_has_pw, we may create a common function for these or let it
duplicated?
--
Matheus Alcantara
v3-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
v3-0002-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
id I think that we could prohibit the
usage of other auth types when use_scram_passthrough is set, what do you think?
--
Matheus Alcantara
Hi, thanks for reviewing this patch!
Em seg., 10 de fev. de 2025 às 20:19, Jacob Champion
escreveu:
>
> On Wed, Jan 22, 2025 at 6:10 AM Matheus Alcantara
> wrote:
> > The attached patch enables SCRAM authentication for dblink connections when
> > using dblink_fdw without r
it looks good to me.
Thanks for the review! v3 with the fixes attached.
--
Matheus Alcantara
v3-0001-Use-read-stream-on-amcheck.patch
Description: Binary data
l.
>
>
> As for 'small values', it means that the average rows is between zero and
> one, to avoid rounding errors and misunderstanding. I think this would be
> ideal.
>
Get it, sounds reasonable to me.
--
Matheus Alcantara
ned but sometimes it can return 1 or more.
> >
> > * There are more spaces than necessary before "If a subplan node ..."
> >
> > * Maybe wrap 'rows' with ?
> >
>
> I agree with the last two points. As for the first one—maybe we could
> simply state that the average rows value can be decimal, especially for
> very small values?
>
I'm just not sure about the "small values"; the 'rows' in decimal will only
happen with small values? What would be a "small value" in this context? My main
point here is more that I think that it would be good to mention *why* the
'rows' can be decimal, not just describe that it could be decimal.
--
Matheus Alcantara
ws
will be returned but sometimes it can return 1 or more.
* There are more spaces than necessary before "If a subplan node ..."
* Maybe wrap 'rows' with ?
--
Matheus Alcantara
-of-rows-and-loops-as-decimal-fraction.patch
> postgres$
Just for reference I'm trying to apply based on commit fb056564ec5.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fb056564ec5bc1c18dd670c963c893cdb6de927e
--
Matheus Alcantara
d-loops-as-decimal-fraction.patch
error: patch failed: src/test/regress/expected/partition_prune.out:3041
error: src/test/regress/expected/partition_prune.out: patch does not apply
--
Matheus Alcantara
://www.postgresql.org/message-id/27b29a35-9b96-46a9-bc1a-914140869...@gmail.com
--
Matheus Alcantara
v1-0002-dblink-Add-SCRAM-pass-through-authentication.patch
Description: Binary data
v1-0001-dblink-refactor-get-connection-routines.patch
Description: Binary data
Em qua., 15 de jan. de 2025 às 14:03, Peter Eisentraut
escreveu:
>
> On 14.01.25 15:14, Matheus Alcantara wrote:
> >> Attached is a fixup patch where I have tried to expand the documentation
> >> a bit in an attempt to clarify how to use this. Maybe check that what
Em ter., 14 de jan. de 2025 às 06:21, Peter Eisentraut
escreveu:
>
> On 09.01.25 16:22, Matheus Alcantara wrote:
> > Yeah, I also think that makes sense.
> >
> > I've made all changes on the attached v2.
>
> (This should probably have been v3, since you had a
uot;)));
>
> Maybe the option of having SCRAM pass-through should be mentioned here?
> It seems sort of analogous to the delegate GSSAPI credentials case.
>
Yeah, I also think that makes sense.
I've made all changes on the attached v2.
--
Matheus Alcantara
v2-0001-postgres_fdw-SCRAM-authentication-pass-through.patch
Description: Binary data
mp;& (mapbts &
> >VISIBILITYMAP_ALL_FROZEN) != 0) ||
> >+ (p->skip_option == SKIP_PAGES_ALL_VISIBLE && (mapbts &
> >VISIBILITYMAP_ALL_VISIBLE) != 0))
> > + continue;
>
> I don't understand this change. The patch aims to be purely applying
>
re it could be benefit
from the AIO project.
--
Matheus Alcantara
v1-0001-Use-read-stream-on-amcheck.patch
Description: Binary data
Plan node, but I
also I'm not sure if this approach makes sense. Attached a simple patch that
play with the idea.
WYT?
[1]
https://www.postgresql.org/message-id/cagpvpcsbn_-t3jvpmmhhsqns2nogo1tsbbyzng1cjggacqj...@mail.gmail.com
--
Matheus Alcantara
From 2577544cc8b93d9a08a872353e3d260c5999ccef Mon Sep 17 00:00:00 2001
tation as well? It only mention insert, update, or delete
operations. Also, I don't know if would be good to link the
PlanForeignModify on this part of the documentation, WYT?
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
both servers and I
got the log message on both servers logs.
Sorry if I misunderstood your message, I probably missed something here.
[1]
https://www.postgresql.org/message-id/9129a012-0415-947e-a68e-59d423071525%40timescale.com
[2] src/backend/libpq/auth-scram.c#read_client_final_message
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
ion.
All usages of this function is using an iteration > 0, so I think that
is just a matter of updating the documentation? If that's the case the
attached patch does that.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
From c560e0c5b7e51e48314c6e3ada6126f9ef27d8e3 Mon Sep 17 00
with a initial documentation, so that we can get
initial thoughts (not sure if I should put the documentation on the
same patch of code changes).
Thanks!
[1]
https://github.com/pgbouncer/pgbouncer/commit/ba1abfe#diff-128a3f9ffa6a6f3863e843089ede6d07010215acf49c66b2d1f1d9baba2f49e7R1001
--
Ma
;
Why was that change made?
Not needed, sorry. Fixed on v2
--
Matheus Alcantara
EDB: https://www.enterprisedb.comFrom 69f41167ab3cf42f5a262403145ae7b53c77a38c Mon Sep 17 00:00:00 2001
From: Matheus Alcantara
Date: Tue, 19 Nov 2024 15:37:57 -0300
Subject: [PATCH v2 1/2] postgres_fdw: SCRAM authe
changes in the coming days.
This patch is based on a previous WIP patch from Peter Eisentraut [1]
[1]
https://github.com/petere/postgresql/commit/90009ccd736e99d65c59b9078d14d76fffc2426a
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
From 65fcb8c9565c7f4ba5c204af775c29c76d474a57 Mon Sep
buf;
...
buf = read_stream_next_buffer(stream, NULL);
...
}
Assert(read_stream_next_buffer(stream, NULL) == InvalidBuffer);
Would this approach make sense on these cases? (this patch and on
collect_corrupt_items)
--
Matheus Alcantara
the autoprewarm
> case; the callback function needs to do additional checks so another
> callback and the use of this variable are required.
Ohh, I see, thanks very much for the explanation.
> v3 is attached.
Thanks.
I don't know if there is another way that this patch could be tested?
Looking
forward on other reviews on this.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
t seems that it is only
written and never read? Just as comparison, the block_range_read_stream_cb
callback used on pg_prewarm seems to not use the per_buffer_data parameter.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
theus AlcantaraFrom 2a2c773b2437b2c491576db8d7ed6b6d1ba2c815 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara
Date: Wed, 12 Jul 2023 18:57:52 -0300
Subject: [PATCH] Remove duplicated LLVMJitHandle->lljit assignment
---
src/backend/jit/llvm/llvmjit.c | 2 --
1 file changed, 2 deletions(-)
diff --g
patched on current master and seems to be working properly.
I'm starting reviewing some patches here, let's see what more experience hackers
has to say about this, but as far I can tell is that is working as expected.
--
Matheus Alcantara
o the
master cleanly. Could you please rebase it?
--
Matheus Alcantara
e table file with no dead space. This
minimizes the size of the table, but can take a long time. It also requires
extra disk space for the new copy of the table, until the operation completes."
https://www.postgresql.org/docs/current/routine-vacuuming.html
--
Matheus Alcantara
lmari
I've tested all 4 of your patches, and all of them seem to work as expected.
This is my first time reviewing a patch, so let's see if more experience
hackers has anything more to say about these patches, but at first they
seem correct to me.
--
Matheus Alcantara
Thanks so much for the answers, I'll try to start looking at some patches.
--
Matheus Alcantara
a=commit;h=6a1f082abac9da756d473e16238a906ca5a592dc
--
Matheus Alcantara
n more. But that concern could be
> alleviated if we put the test somewhere else. Maybe contrib/btree_gist
> would be suitable?
I can't say much about it. If there's anything I can do here, please let
me know.
--
Matheus AlcantaraFrom 9176b605230890f08d9a2d4692dff4fd313746e4 Mon S
The attached patch is failing on make check due to a typo, resubmitting the
correct one.
--
Matheus Alcantaradiff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index a36b4c9c56..b5edc44250 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/exp
I'm studying how the gist index works trying to improve the test coverage of
gistbuild.c.
Reading the source code I noticed that the gistInitBuffering function is not
covered, so I decided to start with it.
Reading the documentation and the source I understood that for this function to
be exec
Hi hackers.
I'm attaching a patch that add some new test cases for tab completion of psql.
This is my first patch that I'm sending here so let me know if I'm doing
something wrong.
--
Matheus AlcantaraFrom 6af6b972960f4e9017f1c311ee4b13a96d5f66a1 Mon Sep 17 00:00:00 200
On Tuesday, December 28th, 2021 at 16:53, Tom lane...@sss.pgh.pa.us wrote:
> Matheus Alcantara msalcantara@pm.me writes:
>
>>> it is not consistent with other \g* commands. Maybe a new statement \senv ?
>>> But what is the use case? You can just press ^z and inside sh
> út 28. 12. 2021 v 19:51 odesílatel Matheus Alcantara
> napsal:
>
>> Hi pgsql hackers, I was testing the new psql command \getenv introduced on
>> commit 33d3eeadb2 and from a user perspective, I think that would be nice if
>> the PSQLVAR parameter were optional
Hi pgsql hackers, I was testing the new psql command \getenv introduced on
commit 33d3eeadb2 and from a user perspective, I think that would be nice if
the PSQLVAR parameter were optional, therefore when it is only necessary to
view the value of the environment variable, the user just run \geten
75 matches
Mail list logo
