Duplicated LLVMJitHandle->lljit assignment?
Hi, I was reading the jit implementation and I notice that the lljit field of LLVMJitHandle is being assigned twice on llvm_compile_module function, is this correct? I'm attaching a supposed fixes that removes the second assignment. I ran meson test and all tests have pass. -- Matheus 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 --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c index 5c30494fa1..09650e2c70 100644 --- a/src/backend/jit/llvm/llvmjit.c +++ b/src/backend/jit/llvm/llvmjit.c @@ -722,8 +722,6 @@ llvm_compile_module(LLVMJitContext *context) elog(ERROR, "failed to JIT module: %s", llvm_error_message(error)); - handle->lljit = compile_orc; - /* LLVMOrcLLJITAddLLVMIRModuleWithRT takes ownership of the module */ } #elif LLVM_VERSION_MAJOR > 6 -- 2.34.1
Re: mprove tab completion for ALTER EXTENSION ADD/DROP
--- Original Message --- On Sunday, November 27th, 2022 at 10:24, vignesh C wrote: > Hi, > > Tab completion for ALTER EXTENSION ADD and DROP was missing, this > patch adds the tab completion for the same. > > Regards, > Vignesh Hi Vignesh I've tested your 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
Re: Make ON_ERROR_STOP stop on shell script failure
--- Original Message --- On Tuesday, November 22nd, 2022 at 20:10, bt22nakamorit wrote: > There was a mistake in the error message for \! so I updated the patch. > > Best, > Tatsuhiro Nakamori Hi I was checking your patch and seems that it failed to be applied into the master cleanly. Could you please rebase it? -- Matheus Alcantara
Re: Index not getting cleaned even though vacuum is running
Hi --- Original Message --- On Tuesday, November 15th, 2022 at 12:38, Karthik Jagadish (kjagadis) wrote: > Hi, > > We notice that vacuum is happening at regular intervals but the space > occupied by indexes is always increasing. Any pointers as to why would this > happen? > > Some outputs below. Auto vacuum is enabled but we notice index size is > growing. > > $ psql -U postgres -d cgms -c "SELECT > pg_size_pretty(SUM(pg_relation_size(table_schema||'.'||table_name))) as size > from information_schema.tables" > > size > > --- > > 25 GB > > (1 row) > > $ psql -U postgres -d cgms -c "SELECT > pg_size_pretty(SUM(pg_indexes_size(table_schema||'.'||table_name) + > pg_relation_size(table_schema||'.'||table_name))) as size from > information_schema.tables" > > size > > > > 151 GB > > (1 row) > > $ sudo du -hsc /var/lib/pgsql/12/data > > 154G /var/lib/pgsql/12/data > > 154G total > > Appreciate if someone can give some pointers. > > Regards, > > Karthik As far as I know vacuum just mark the space of dead rows available for future reuse, so I think it's expected that the size doesn't decrease. "The standard form of VACUUM removes dead row versions in tables and indexes and marks the space available for future reuse. However, it will not return the space to the operating system, except in the special case where one or more pages at the end of a table become entirely free and an exclusive table lock can be easily obtained. In contrast, VACUUM FULL actively compacts tables by writing a complete new version of the 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
Re: [PATCH] Improve tab completion for ALTER TABLE on identity columns
> Hi Hackers, > > I noticed that psql has no tab completion around identity columns in > ALTER TABLE, so here's some patches for that. > > In passing, I also added completion for ALTER SEQUECNE … START, which was > missing for some reason. > > - ilmari Hi ilmari 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
Re: Interesting areas for beginners
Thanks so much for the answers, I'll try to start looking at some patches. -- Matheus Alcantara
Interesting areas for beginners
Hi hackers. Can you please share some areas that would be good to start contributing? Some months ago I've got my first patch accept [1], and I'm looking to try to make other contributions. Thanks in advance! [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6a1f082abac9da756d473e16238a906ca5a592dc -- Matheus Alcantara
Re: Trying to add more tests to gistbuild.c
--- Original Message --- On Friday, July 29th, 2022 at 19:53, Tom Lane t...@sss.pgh.pa.us wrote: > I wonder if we can combine ideas from the two patches to get a > better tradeoff of code coverage vs. runtime. I was checking the Pavel patch and notice that he was using the fillfactor parameter when creating the gist index. I changed my previous patch to include this parameter and the code coverage of gistbuild.c and gistbuildbuffers.c was improved to 97.7% and 92.8% respectively. I'm attaching this new patch, could you please check if this change make sense and also don't impact the test runtime? > Another thing we might consider is to move the testing responsibility > somewhere else. The reason I'm allergic to adding a lot of runtime > here is that the core regression tests are invoked at least four times > in a standard buildfarm run, often 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 Sep 17 00:00:00 2001 From: Matheus Alcantara Date: Sat, 30 Jul 2022 15:38:05 -0300 Subject: [PATCH] Improve test coverage of gist build --- src/test/regress/expected/gist.out | 6 ++ src/test/regress/sql/gist.sql | 9 + 2 files changed, 15 insertions(+) diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out index a36b4c9c56..c024e5bbff 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -387,6 +387,12 @@ select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; ERROR: lossy distance functions are not supported in index-only scans +-- Build an index using buffering caused by a index build that don't fit on cache. +set effective_cache_size = '1MB'; +create index gist_tbl_box_index_buffering on gist_tbl using gist (p, b, c); +reset effective_cache_size; +-- Force an index build using buffering. +create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on, fillfactor=50); -- Clean up reset enable_seqscan; reset enable_bitmapscan; diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 3360266370..6d9918cf04 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -169,6 +169,15 @@ explain (verbose, costs off) select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; + +-- Build an index using buffering caused by a index build that don't fit on cache. +set effective_cache_size = '1MB'; +create index gist_tbl_box_index_buffering on gist_tbl using gist (p, b, c); +reset effective_cache_size; + +-- Force an index build using buffering. +create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on, fillfactor=50); + -- Clean up reset enable_seqscan; reset enable_bitmapscan; -- 2.37.1
Re: Trying to add more tests to gistbuild.c
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/expected/gist.out @@ -387,6 +387,12 @@ select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; ERROR: lossy distance functions are not supported in index-only scans +-- Build an index using buffering caused by a index build that don't fit on cache. +set effective_cache_size = '1MB'; +create index gist_tbl_box_index_buffering on gist_tbl using gist (p, b, c); +reset effective_cache_size; +-- Force an index build using buffering. +create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on); -- Clean up reset enable_seqscan; reset enable_bitmapscan; diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 3360266370..214366c157 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -169,6 +169,15 @@ explain (verbose, costs off) select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1; + +-- Build an index using buffering caused by a index build that don't fit on cache. +set effective_cache_size = '1MB'; +create index gist_tbl_box_index_buffering on gist_tbl using gist (p, b, c); +reset effective_cache_size; + +-- Force an index build using buffering. +create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on); + -- Clean up reset enable_seqscan; reset enable_bitmapscan;
Trying to add more tests to gistbuild.c
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 executed it is necessary to force bufferring=on when creating the index or the index to be created is big enough to not fit in the cache, am I correct? Considering the above, I added two new index creation statements in the gist regression test (patch attached) to create an index using buffering=on and another to try to simulate an index that does not fit in the cache. With these new tests the coverage went from 45.3% to 85.5%, but I have some doubts: - Does this test make sense? - Would there be a way to validate that the buffering was done correctly? - Is this test necessary? I've been studying Postgresql implementations and I'm just trying to start contributing the source code. -- Matheus Alcantaradiff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out index a36b4c9c56..044986433a 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -46,6 +46,12 @@ vacuum analyze gist_tbl; set enable_seqscan=off; set enable_bitmapscan=off; set enable_indexonlyscan=on; +-- Build an index using buffering caused by a index build that don't fit on cache. +set effective_cache_size = '1MB'; +create index gist_tbl_box_index_buffering on gist_tbl using gist (p, b, c); +reset effective_cache_size; +-- Force a index build using buffering. +create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on); -- Test index-only scan with point opclass create index gist_tbl_point_index on gist_tbl using gist (p); -- check that the planner chooses an index-only scan diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 3360266370..836ce84d71 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -55,6 +55,14 @@ set enable_seqscan=off; set enable_bitmapscan=off; set enable_indexonlyscan=on; +-- Build an index using buffering caused by a index build that don't fit on cache. +set effective_cache_size = '1MB'; +create index gist_tbl_box_index_buffering on gist_tbl using gist (p, b, c); +reset effective_cache_size; + +-- Force an index build using buffering. +create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on); + -- Test index-only scan with point opclass create index gist_tbl_point_index on gist_tbl using gist (p);
[PATCH] Add tests for psql tab completion
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 2001
From: Matheus Alcantara
Date: Sat, 12 Feb 2022 16:45:55 -0300
Subject: [PATCH] psql: Add tests for tab completion
---
src/bin/psql/t/010_tab_completion.pl | 92
1 file changed, 92 insertions(+)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index 005961f34d..f74ff96226 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -45,6 +45,7 @@ $node->safe_psql('postgres',
. "CREATE TABLE mytab246 (f1 int, f2 text);\n"
. "CREATE TABLE \"mixedName\" (f1 int, f2 text);\n"
. "CREATE TYPE enum1 AS ENUM ('foo', 'bar', 'baz', 'BLACK');\n"
+ . "CREATE INDEX idx_tab1_c1 ON tab1(c1);\n"
. "CREATE PUBLICATION some_publication;\n");
# Developers would not appreciate this test adding a bunch of junk to
@@ -382,6 +383,97 @@ check_completion(
clear_query();
+check_completion(
+ "CREATE OR REPLACE \t\t",
+ qr/AGGREGATE +LANGUAGE +RULE +TRIGGER +\r\nFUNCTION +PROCEDURE +TRANSFORM +VIEW/,
+ "check create or replace");
+
+clear_query();
+
+check_completion(
+ "ALTER FOREIGN \t\t",
+ qr/DATA WRAPPER +TABLE/ ,
+ "check alter foreign");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX \t\t",
+ qr/ALL IN TABLESPACE +information_schema. +tab1_pkey\r\nidx_tab1_c1/,
+ "check alter index");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 \t\t",
+ qr/ALTER COLUMN +NO DEPENDS ON EXTENSION +RESET\r\nATTACH PARTITION +OWNER TO +SET\r\nDEPENDS ON EXTENSION +RENAME TO/,
+ "check alter index ");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 ATTACH \t\t",
+ qr/PARTITION/,
+ "check alter index attach");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 ATTACH PARTITION \t\t",
+ qr/idx_tab1_c1 +public. +\r\ninformation_schema. +tab1_pkey/,
+ "check alter index ATTACH PARTITION");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 ALTER \t\t",
+ qr/COLUMN/,
+ "check alter index alter");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 ALTER COLUMN \t\t",
+ qr/1 +SET +STATISTICS/,
+ "check alter index alter column");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 ALTER COLUMN 1 SET \t\t",
+ qr/STATISTICS/,
+ "check alter index alter column set");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 SET \t\t",
+ qr/\( +TABLESPACE/,
+ "check alter index set");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 RESET \t\t",
+ qr/\( +\a/,
+ "check alter index reset");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 NO DEPENDS \t\t",
+ qr/ON EXTENSION/,
+ "check alter index no depends");
+
+clear_query();
+
+check_completion(
+ "ALTER INDEX idx_tab1_c1 DEPENDS \t\t",
+ qr/ON EXTENSION/,
+ "check alter index depends");
+
+clear_query();
+
# check VersionedQuery infrastructure
check_completion(
"DROP PUBLIC\t \t\t",
--
2.35.1
Re: [PROPOSAL] Make PSQLVAR on \getenv opitional
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 shell write echo >>> $xxx, and then fg > >> I think that the basic use case would be just for debugging, instead call >> \getenv and them \echo, we could just use \getenv. I don't see any other >> advantages, It would just be to >> >> write fewer commands. I think that ^z and then fg is a good alternative, >> since this behavior would be inconsistent. > > You don't even need to do that much. This works fine: > > postgres=# \! echo $PATH > > So I'm not convinced that we need another way to spell that. > > (Admittedly, this probably doesn't work on Windows, but > > I gather that environment variables are less interesting there.) > > regards, tom lane I definitely agree with this. We already have other ways to handle it. Thanks for discussion and quick responses. Matheus Alcantara
Re: [PROPOSAL] Make PSQLVAR on \getenv opitional
> ú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, therefore when it is only necessary to >> view the value of the environment variable, the user just run \getenv, for >> example: >> >> \getenv PATH >> /usr/local/sbin:/usr/local/bin:/usr/bin >> >> And when it is necessary to assign the environment variable in a variable, >> the user could execute like this: >> >> \getenv PATH myvar >> \echo :myvar >> /usr/local/sbin:/usr/local/bin:/usr/bin >> >> For this flexibility the order of parameters would need to be reversed, >> instead of \getenv PSQLVAR ENVVAR would be \getenv ENVVAR PSQLVAR. >> >> What do you guys think? I'm not a C expert but if this proposal is >> interesting I can write a patch. > > 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 shell write echo > $xxx, and then fg I think that the basic use case would be just for debugging, instead call \getenv and them \echo, we could just use \getenv. I don't see any other advantages, It would just be to write fewer commands. I think that ^z and then fg is a good alternative, since this behavior would be inconsistent. > Regards > > Pavel > >> This is my first time sending an email here, so let me know if I doing >> something wrong.
[PROPOSAL] Make PSQLVAR on \getenv opitional
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 \getenv, for example: \getenv PATH /usr/local/sbin:/usr/local/bin:/usr/bin And when it is necessary to assign the environment variable in a variable, the user could execute like this: \getenv PATH myvar \echo :myvar /usr/local/sbin:/usr/local/bin:/usr/bin For this flexibility the order of parameters would need to be reversed, instead of \getenv PSQLVAR ENVVAR would be \getenv ENVVAR PSQLVAR. What do you guys think? I'm not a C expert but if this proposal is interesting I can write a patch. This is my first time sending an email here, so let me know if I doing something wrong.
