Re: Postgresql13_beta1 (could not rename temporary statistics file) Windows 64bits
Em dom., 14 de jun. de 2020 às 23:08, Michael Paquier escreveu: > On Fri, Jun 12, 2020 at 03:15:52PM -0300, Ranier Vilela wrote: > > Posgres13_beta1, is consistently writing to the logs, "could not rename > > temporary statistics file". > > When analyzing the source that writes the log, I simplified the part that > > writes the logs a little. > > FWIW, I have been running a server on Windows for some time with > pgbench running in the background, combined with some starts and stops > but I cannot see that. This log entry uses LOG, which is the level we > use for the TAP tests and please note that there are four buildfarm > animals for Windows able to run the TAP tests and they don't seem to > show that problem either: drongo, fairywen, jacana and bowerbird. I > may be missing something of course Hi Michael, thsnks for answer. Yeah, something is wrong with Postgres and Windows 10 (not server) with msvc 2019 (64 bits) II already reported on another thread, that vcregress is failing with (float8 and partitionprune) and now these messages are showing up. None buildfarm animal, have that combination, but as Postgres officially supports it .. regards, Ranier Vilela <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail> Livre de vírus. www.avast.com <https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>. <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
Re: Postgresql13_beta1 (could not rename temporary statistics file) Windows 64bits
posix rename, "renames a file, moving it between directories if required". pgrename, win32 port uses MoveFileEx, to support rename files at Windows side, but, actually don't allow "renames a file, moving it between directories if required". To match the same characteristics as posix rename, we need to add a flag to MoveFileEx (MOVEFILE_COPY_ALLOWED) Which allows, if necessary, to move between volumes, drives and directories. Such a resource seems to decrease the chances of occurring, permission denied, when renaming the temporary statistics file. allow_win32_rename_across_volumes.patch Description: Binary data
Re: Postgresql13_beta1 (could not rename temporary statistics file) Windows 64bits
Em sex., 12 de jun. de 2020 às 15:15, Ranier Vilela escreveu: > Posgres13_beta1, is consistently writing to the logs, "could not rename > temporary statistics file". > When analyzing the source that writes the log, I simplified the part that > writes the logs a little. > > 1. I changed from if else if to if > 2. For the user, better to have more errors recorded, which can help in > discovering the problem > 3. Errors are independent of each other > 4. If I can't release tmpfile, there's no way to delete it (unlink) > 5. If I can rename, there is no need to delete it (unlink) tmpfile. > Fix for case 5. 001_simplifies_write_statsfiles_v2.patch Description: Binary data
Postgresql13_beta1 (could not rename temporary statistics file) Windows 64bits
Posgres13_beta1, is consistently writing to the logs, "could not rename temporary statistics file". When analyzing the source that writes the log, I simplified the part that writes the logs a little. 1. I changed from if else if to if 2. For the user, better to have more errors recorded, which can help in discovering the problem 3. Errors are independent of each other 4. If I can't release tmpfile, there's no way to delete it (unlink) 5. If I can rename, there is no need to delete it (unlink) tmpfile. Attached is the patch that proposes these changes. Now, the problem has not been solved. 1. statfile, is it really closed or does it not exist in the directory? There is no way to rename a file, which is open and in use. 2. Why delete (pgstat_stat_filename), if permanent is true: const char * statfile = permanent? PGSTAT_STAT_PERMANENT_FILENAME: pgstat_stat_filename; statfile is PGSTAT_STAT_PERMANENT_FILENAME and not pgstat_stat_filename regards, Ranier Vilela 001_simplifies_write_statsfiles.patch Description: Binary data
Re: [PATCH] fix two shadow vars (src/backend/commands/sequence.c)
Em qui., 11 de jun. de 2020 às 19:54, Tom Lane escreveu: > Ranier Vilela writes: > > elog and errmsg_internal, permits use as proposed by the patch, > > does it mean that errmsg, does not allow and does not do the same job as > > snprintf? > > Yes. errmsg() strings are captured for translation. If they contain > platform-dependent substrings, that's a problem, because only one variant > will get captured. And INT64_FORMAT is platform-dependent. > > We have of late decided that it's safe to use %lld (or %llu) to format > int64s everywhere, but you then have to cast the printf argument to > match that explicitly. See commit 6a1cd8b92 for precedent. > Hi Tom, thank you for the detailed explanation. I see commit 6a1cd8b92, and I think which is the same case with basebackup.c (total_checksum_failures), maxv and minv, are int64 (INT64_FORMAT). %lld -> (long long int) maxv %lld -> (long long int) minv Attached new patch, with fixes from commit 6a1cd8b92. regards, Ranier Vilela > > regards, tom lane > fix_shadows_buf_var_v2.patch Description: Binary data
Re: [PATCH] fix two shadow vars (src/backend/commands/sequence.c)
Em qui., 11 de jun. de 2020 às 17:19, Alvaro Herrera <
alvhe...@2ndquadrant.com> escreveu:
> On 2020-Jun-11, Ranier Vilela wrote:
>
> > Hi,
> > src/backend/commands/sequence.c
> > Has two shadows (buf var), with two unnecessary variables declared.
>
> These are not unnecessary -- removing them breaks translatability of
> those messages. If these were ssize_t you could use '%zd' (see commit
> ac4ef637ad2f) but I don't think you can in this case.
>
Hi Alvaro, thanks for reply.
File backend\utils\sort\tuplesort.c:
elog(LOG, "worker %d using " INT64_FORMAT " KB of memory for read buffers
among %d input tapes",
File backend\storage\ipc\shm_toc.c:
elog(ERROR, "could not find key " UINT64_FORMAT " in shm TOC at %p",
File backend\storage\large_object\inv_api.c:
* use errmsg_internal here because we don't want to expose INT64_FORMAT
errmsg_internal("invalid large object seek target: " INT64_FORMAT,
elog and errmsg_internal, permits use as proposed by the patch,
does it mean that errmsg, does not allow and does not do the same job as
snprintf?
regards,
Ranier Vilela
Re: Windows regress fails (latest HEAD)
Em qui., 11 de jun. de 2020 às 11:00, Andrew Dunstan < andrew.duns...@2ndquadrant.com> escreveu: > > On 6/11/20 9:40 AM, Ranier Vilela wrote: > > Em qui., 11 de jun. de 2020 às 10:36, Andrew Dunstan > > > <mailto:andrew.duns...@2ndquadrant.com>> escreveu: > > > > > > > > > > > > > > > > * which configuration settings you're using > > > > > > None. Only build with default configuration (release is default). > > > > > > We would also need to see the contents of your config.pl > > <http://config.pl> > > > > It seems that I am missing something, there is no config.pl > > <http://config.pl>, anywhere in the postgres directory. > > > > If there isn't one it uses config_default.pl. > I see. As I did a clean build, from github (git clone), there is no config.pl, so, was using the same file. ( https://github.com/postgres/postgres/blob/master/src/tools/msvc/config_default.pl ) regards, Ranier VIlela
Re: Windows regress fails (latest HEAD)
Em qui., 11 de jun. de 2020 às 10:36, Andrew Dunstan < andrew.duns...@2ndquadrant.com> escreveu: > > On 6/11/20 9:28 AM, Ranier Vilela wrote: > > Em qui., 11 de jun. de 2020 às 10:01, Andrew Dunstan > > > <mailto:andrew.duns...@2ndquadrant.com>> escreveu: > > > > > > On 6/11/20 8:52 AM, Ranier Vilela wrote: > > > Hi, > > > Latest HEAD, fails with windows regress tests. > > > > > > float8 ... FAILED 517 ms > > > partition_prune ... FAILED 3085 ms > > > > > > > > > > The first thing you should do when you find this is to see if > > there is a > > buildfarm report of the failure. If there isn't then try to work > > out why. > > > > Sorry, I will have to research the buildfarm, I have no reference to it. > > > > > See https://buildfarm.postgresql.org Ok. > > > > > > * which configuration settings you're using > > > > None. Only build with default configuration (release is default). > > > We would also need to see the contents of your config.pl It seems that I am missing something, there is no config.pl, anywhere in the postgres directory. > > > > > > > * what are the regression differences you get in the failing tests > > > > It was sent as an attachment. > > > > > > If you send attachments please refer to them in the body of your email, > otherwise they are easily missed, as I did. > Ah yes, ok, I'll remember that. regards, Ranier Vilela
Re: Windows regress fails (latest HEAD)
Em qui., 11 de jun. de 2020 às 10:01, Andrew Dunstan < andrew.duns...@2ndquadrant.com> escreveu: > > On 6/11/20 8:52 AM, Ranier Vilela wrote: > > Hi, > > Latest HEAD, fails with windows regress tests. > > > > float8 ... FAILED 517 ms > > partition_prune ... FAILED 3085 ms > > > > > > The first thing you should do when you find this is to see if there is a > buildfarm report of the failure. If there isn't then try to work out why. > Sorry, I will have to research the buildfarm, I have no reference to it. > > Also, when making a report like this, it is essential to let us know > things like: > > * which commit is causing the failure (git bisect is good for finding > this) > Thanks for hit (git bisect). > * what Windows version you're testing on > Windows 10 (2004) * which compiler you're using > msvc 2019 (64 bits) * which configuration settings you're using > None. Only build with default configuration (release is default). * what are the regression differences you get in the failing tests > It was sent as an attachment. regards, Ranier Vilela
Windows regress fails (latest HEAD)
Hi, Latest HEAD, fails with windows regress tests. float8 ... FAILED 517 ms partition_prune ... FAILED 3085 ms regards, Ranier VIlela regression.diffs Description: Binary data regression.out Description: Binary data
[PATCH] fix two shadow vars (src/backend/commands/sequence.c)
Hi, src/backend/commands/sequence.c Has two shadows (buf var), with two unnecessary variables declared. For readability reasons, the declaration of variable names in the prototypes was also corrected. regards, Ranier Vilela fix_shadows_buf_var.patch Description: Binary data
Re: Speedup usages of pg_*toa() functions
Em ter., 9 de jun. de 2020 às 17:42, Andrew Gierth <
and...@tao11.riddles.org.uk> escreveu:
> >>>>> "Ranier" == Ranier Vilela writes:
>
> Ranier> So I would change, just the initialization (var uvalue), even
> though it is
> Ranier> irrelevant.
>
> Ranier> int
> Ranier> pg_lltoa(int64 value, char *a)
> Ranier> {
> Ranier> int len = 0;
> Ranier> uint64 uvalue;
>
> Ranier> if (value < 0)
> Ranier> {
> Ranier> uvalue = (uint64) 0 - uvalue;
>
> Use of uninitialized variable.
>
Sorry, my mistake.
uvalue = (uint64) 0 - value;
regards,
Ranier Vilela
>
> --
> Andrew (irc:RhodiumToad)
>
Re: Speedup usages of pg_*toa() functions
Em ter., 9 de jun. de 2020 às 15:53, Andrew Gierth <
and...@tao11.riddles.org.uk> escreveu:
> >>>>> "Ranier" == Ranier Vilela writes:
>
> Ranier> Where " ends up with two copies of pg_ultoa_n inlined into it",
> Ranier> in this simplified example?
>
> The two references to sprintf are both inlined copies of your pg_utoa.
>
> Ranier> (b) I call this tail cut, I believe it saves time, for sure.
>
> You seem to have missed the point that the pg_ultoa_n / pg_ulltoa_n
> functions DO NOT ADD A TRAILING NUL. Which means that pg_ltoa / pg_lltoa
> can't just tail call them, since they must add the NUL after.
>
> Ranier> Regarding bugs:
>
> Ranier> (c) your version don't check size of a var, when pg_ulltoa_n
> Ranier> warning about "least MAXINT8LEN bytes."
>
> Ranier> So in theory, I could blow it up, by calling pg_lltoa.
>
> No. Callers of pg_lltoa are required to provide a buffer of at least
> MAXINT8LEN+1 bytes.
>
Thanks for explanations.
So I would change, just the initialization (var uvalue), even though it is
irrelevant.
int
pg_lltoa(int64 value, char *a)
{
int len = 0;
uint64 uvalue;
if (value < 0)
{
uvalue = (uint64) 0 - uvalue;
a[len++] = '-';
}
else
uvalue = value;
len += pg_ulltoa_n(uvalue, a + len);
a[len] = '\0';
return len;
}
regards,
Ranier Vilela
Re: Speedup usages of pg_*toa() functions
Em ter., 9 de jun. de 2020 às 13:01, Andrew Gierth <
and...@tao11.riddles.org.uk> escreveu:
> >>>>> "Ranier" == Ranier Vilela writes:
>
> Ranier> Written like that, wouldn't it get better?
>
> Ranier> int
> Ranier> pg_lltoa(int64 value, char *a)
> Ranier> {
> Ranier> if (value < 0)
> Ranier> {
> Ranier> int len = 0;
> Ranier> uint64 uvalue = (uint64) 0 - uvalue;
> Ranier> a[len++] = '-';
> Ranier> len += pg_ulltoa_n(uvalue, a + len);
> Ranier> a[len] = '\0';
> Ranier> return len;
> Ranier> }
> Ranier> else
> Ranier> return pg_ulltoa_n(value, a);
> Ranier> }
>
> No. While it doesn't matter so much for pg_lltoa since that's unlikely
> to inline multiple pg_ulltoa_n calls, if you do pg_ltoa like this it (a)
> ends up with two copies of pg_ultoa_n inlined into it, and (b) you don't
> actually save any useful amount of time. Your version is also failing to
> add the terminating '\0' for the positive case and has other obvious
> bugs.
>
(a) Sorry, I'm not asm specialist.
#include
int pg_utoa(unsigned int num, char * a) {
int len;
len = sprintf(a, "%lu", num);
return len;
}
int pg_toa(int num, char * a)
{
if (num < 0) {
int len;
len = pg_utoa(num, a);
a[len] = '\0';
return len;
}
else
return pg_utoa(num, a);
}
.LC0:
.string "%lu"
pg_utoa(unsigned int, char*):
mov edx, edi
xor eax, eax
mov rdi, rsi
mov esi, OFFSET FLAT:.LC0
jmp sprintf
pg_toa(int, char*):
pushrbp
testedi, edi
mov rbp, rsi
mov edx, edi
mov esi, OFFSET FLAT:.LC0
mov rdi, rbp
mov eax, 0
js .L7
pop rbp
jmp sprintf
.L7:
callsprintf
movsx rdx, eax
mov BYTE PTR [rbp+0+rdx], 0
pop rbp
ret
Where " ends up with two copies of pg_ultoa_n inlined into it", in this
simplified example?
(b) I call this tail cut, I believe it saves time, for sure.
Regarding bugs:
(c) your version don't check size of a var, when pg_ulltoa_n
warning about "least MAXINT8LEN bytes."
So in theory, I could blow it up, by calling pg_lltoa.
(d) So I can't trust pg_ulltoa_n, when var a, is it big enough?
If not, there are more bugs.
regards,
Ranier Vilela
Re: Speedup usages of pg_*toa() functions
Em ter., 9 de jun. de 2020 às 07:55, David Rowley
escreveu:
> On Tue, 9 Jun 2020 at 22:08, Andrew Gierth
> wrote:
> >
> > >>>>> "David" == David Rowley writes:
> >
> > David> This allows us to speed up a few cases. int2vectorout() should
> > David> be faster and int8out() becomes a bit faster if we get rid of
> > David> the strdup() call and replace it with a palloc()/memcpy() call.
> >
> > What about removing the memcpy entirely? I don't think we save anything
> > much useful here by pallocing the exact length, rather than doing what
> > int4out does and palloc a fixed size and convert the int directly into
> > it.
>
> On looking back through git blame, it seems int2out and int4out have
> been that way since at least 1996, before int8.c existed. int8out has
> been doing it since fa838876e9f -- Include 8-byte integer type. dated
> 1998. Quite likely the larger than required palloc size back then was
> more of a concern. So perhaps you're right about just doing it that
> way instead. With that and the ints I tested with, the int8
> performance should be about aligned to int4 performance.
>
> > For pg_ltoa, etc., I don't like adding the extra call to pg_ultoa_n - at
> > least on my clang, that results in two copies of pg_ultoa_n inlined.
> > How about doing it like,
> >
> > int
> > pg_lltoa(int64 value, char *a)
> > {
> > int len = 0;
> > uint64 uvalue = value;
> >
> > if (value < 0)
> > {
> > uvalue = (uint64) 0 - uvalue;
> > a[len++] = '-';
> > }
> > len += pg_ulltoa_n(uvalue, a + len);
> > a[len] = '\0';
> > return len;
> > }
>
> Written like that, wouldn't it get better?
int
pg_lltoa(int64 value, char *a)
{
if (value < 0)
{
int len = 0;
uint64 uvalue = (uint64) 0 - uvalue;
a[len++] = '-';
len += pg_ulltoa_n(uvalue, a + len);
a[len] = '\0';
return len;
}
else
return pg_ulltoa_n(value, a);
}
regards,
Ranier Vilela
Re: PostgresSQL 13.0 Beta 1 on Phoronix
Em seg., 25 de mai. de 2020 às 03:57, Michael Paquier escreveu: > On Sun, May 24, 2020 at 02:50:08PM -0300, Ranier Vilela wrote: > > Em dom., 24 de mai. de 2020 às 14:34, Peter Geoghegan > escreveu: > >> It looks like they're only running pgbench for 60 second runs in all > >> configurations -- notice that "-T 60" is passed to pgbench. I'm not > >> entirely sure that that's all that there is to it. Still, there isn't > >> any real attempt to make it clear what's going on here. I have my > >> doubts about how representative these numbers are for that reason. > > > > I also find it very suspicious. > > I don't know, but what seems pretty clear to me is this benchmark does > zero customization of postgresql.conf (it disables autovacuum!?), and > that the number of connections is calculated based on the number of > cores while the scaling factor is visibly calculated from the amount > of memory available in the environment. Perhaps the first part is > wanted, but we are very conservative to allow PG to work on small-ish > machines with the default configuration, and a 56-core machine with > 378GB of memory is not something I would define as small-ish. > Does this mean that V13 would need additional settings in postgresql.conf, to perform better than V12, out of the box? If there is any new feature in V13 that needs some configuration in postgresql.conf, Would be bettert it should be documented or configured in the installation itself, to avoid this type of misunderstanding, which harms the perception of Postgres. regards, Ranier Vilela
Re: PostgresSQL 13.0 Beta 1 on Phoronix
Em dom., 24 de mai. de 2020 às 14:34, Peter Geoghegan escreveu: > On Sun, May 24, 2020 at 2:52 AM Ranier Vilela wrote: > > There was news in Phoronix about the Beta 1 Release of Postgres (1). > > Unfortunately for Postgres advocacy it does not bring good news, > > it is showing regressions in the benchmarks compared to version 12. > > Without going into the technical merits of how the test was done, > > they have no way of knowing whether such regressions actually exist or > if it is a failure of how the tests were done. > > This shellscript appears to be used by Phoronix to run pgbench: > > > https://github.com/phoronix-test-suite/phoronix-test-suite/blob/f0f8c726f2700faea363f176a4b28dab026d45d0/ob-cache/test-profiles/pts/pgbench-1.8.4/install.sh > > It looks like they're only running pgbench for 60 second runs in all > configurations -- notice that "-T 60" is passed to pgbench. I'm not > entirely sure that that's all that there is to it. Still, there isn't > any real attempt to make it clear what's going on here. I have my > doubts about how representative these numbers are for that reason. > I also find it very suspicious.V12 seems to be better at read-only workloads (at least it shows the graphics). I'm using V13, in normal mode (read / write), medium load. regards, Ranier VIlela
PostgresSQL 13.0 Beta 1 on Phoronix
Hi, There was news in Phoronix about the Beta 1 Release of Postgres (1). Unfortunately for Postgres advocacy it does not bring good news, it is showing regressions in the benchmarks compared to version 12. Without going into the technical merits of how the test was done, they have no way of knowing whether such regressions actually exist or if it is a failure of how the tests were done. But it would be nice to have arguments to counter, for the sake of Postgres' promotion. I'm using the development version (latest), and for now, it seems to be faster than version 12. regards, Ranier Vilela 1. https://www.phoronix.com/scan.php?page=news_item&px=PostgreSQL-13-Beta
Re: Parallel Seq Scan vs kernel read ahead
Em qua., 20 de mai. de 2020 às 21:03, Thomas Munro escreveu: > On Thu, May 21, 2020 at 11:51 AM Ranier Vilela > wrote: > > Em qua., 20 de mai. de 2020 às 20:48, Thomas Munro < > thomas.mu...@gmail.com> escreveu: > >> On Thu, May 21, 2020 at 11:15 AM Ranier Vilela > wrote: > >> > postgres=# set max_parallel_workers_per_gather = 0; > >> > Time: 227238,445 ms (03:47,238) > >> > postgres=# set max_parallel_workers_per_gather = 1; > >> > Time: 138027,351 ms (02:18,027) > >> > >> Ok, so it looks like NT/NTFS isn't suffering from this problem. > >> Thanks for testing! > > > > Maybe it wasn’t clear, the tests were done with your patch applied. > > Oh! And how do the times look without it? > Vanila Postgres (latest) create table t as select generate_series(1, 8)::int i; set max_parallel_workers_per_gather = 0; Time: 210524,317 ms (03:30,524) set max_parallel_workers_per_gather = 1; Time: 146982,737 ms (02:26,983) regards, Ranier Vilela
Re: Parallel Seq Scan vs kernel read ahead
Em qua., 20 de mai. de 2020 às 20:48, Thomas Munro escreveu: > On Thu, May 21, 2020 at 11:15 AM Ranier Vilela > wrote: > > postgres=# set max_parallel_workers_per_gather = 0; > > Time: 227238,445 ms (03:47,238) > > postgres=# set max_parallel_workers_per_gather = 1; > > Time: 138027,351 ms (02:18,027) > > Ok, so it looks like NT/NTFS isn't suffering from this problem. > Thanks for testing! > Maybe it wasn’t clear, the tests were done with your patch applied. regards, Ranier Vilela
Re: Parallel Seq Scan vs kernel read ahead
Em qua., 20 de mai. de 2020 às 18:49, Thomas Munro escreveu: > On Wed, May 20, 2020 at 11:03 PM Ranier Vilela > wrote: > > Time: 47767,916 ms (00:47,768) > > Time: 32645,448 ms (00:32,645) > > Just to make sure kernel caching isn't helping here, maybe try making > the table 2x or 4x bigger? My test was on a virtual machine with only > 4GB RAM, so the table couldn't be entirely cached. > 4x bigger. Postgres defaults settings. postgres=# create table t as select generate_series(1, 8)::int i; SELECT 8 postgres=# \timing Timing is on. postgres=# set max_parallel_workers_per_gather = 0; SET Time: 8,622 ms postgres=# select count(*) from t; count --- 8 (1 row) Time: 227238,445 ms (03:47,238) postgres=# set max_parallel_workers_per_gather = 1; SET Time: 20,975 ms postgres=# select count(*) from t; count --- 8 (1 row) Time: 138027,351 ms (02:18,027) regards, Ranier Vilela
Re: Parallel Seq Scan vs kernel read ahead
Em qua., 20 de mai. de 2020 às 00:09, Thomas Munro escreveu: > On Wed, May 20, 2020 at 2:23 PM Amit Kapila > wrote: > > Good experiment. IIRC, we have discussed a similar idea during the > > development of this feature but we haven't seen any better results by > > allocating in ranges on the systems we have tried. So, we want with > > the current approach which is more granular and seems to allow better > > parallelism. I feel we need to ensure that we don't regress > > parallelism in existing cases, otherwise, the idea sounds promising to > > me. > > Yeah, Linux seems to do pretty well at least with smallish numbers of > workers, and when you use large numbers you can probably tune your way > out of the problem. ZFS seems to do fine. I wonder how well the > other OSes cope. > Windows 10 (64bits, i5, 8GB, SSD) postgres=# set max_parallel_workers_per_gather = 0; SET Time: 2,537 ms postgres=# select count(*) from t; count --- 2 (1 row) Time: 47767,916 ms (00:47,768) postgres=# set max_parallel_workers_per_gather = 1; SET Time: 4,889 ms postgres=# select count(*) from t; count --- 2 (1 row) Time: 32645,448 ms (00:32,645) How display " -> execution time 5.2s, average read size ="? regards, Ranier VIlela
Re: Why is pq_begintypsend so slow?
Em seg., 18 de mai. de 2020 às 13:38, Tom Lane escreveu: > Andres Freund writes: > >> FWIW, I've also observed, in another thread (the node func generation > >> thing [1]), that inlining enlargeStringInfo() helps a lot, especially > >> when inlining some of its callers. Moving e.g. appendStringInfo() inline > >> allows the compiler to sometimes optimize away the strlen. But if > >> e.g. an inlined appendBinaryStringInfo() still calls enlargeStringInfo() > >> unconditionally, successive appends cannot optimize away memory accesses > >> for ->len/->data. > > > With a set of patches doing so, int4send itself is not a significant > > factor for my test benchmark [1] anymore. > > This thread seems to have died out, possibly because the last set of > patches that Andres posted was sufficiently complicated and invasive > that nobody wanted to review it. I thought about this again after > seeing that [1] is mostly about pq_begintypsend overhead, and had > an epiphany: there isn't really a strong reason for pq_begintypsend > to be inserting bits into the buffer at all. The bytes will be > filled by pq_endtypsend, and nothing in between should be touching > them. So I propose 0001 attached. It's poking into the stringinfo > abstraction a bit more than I would want to do if there weren't a > compelling performance reason to do so, but there evidently is. > > With 0001, pq_begintypsend drops from being the top single routine > in a profile of a test case like [1] to being well down the list. > The next biggest cost compared to text-format output is that > printtup() itself is noticeably more expensive. A lot of the extra > cost there seems to be from pq_sendint32(), which is getting inlined > into printtup(), and there probably isn't much we can do to make that > cheaper. But eliminating a common subexpression as in 0002 below does > help noticeably, at least with the rather old gcc I'm using. > Again, I see problems with the types declared in Postgres. 1. pq_sendint32 (StringInfo buf, uint32 i) 2. extern void pq_sendbytes (StringInfo buf, const char * data, int datalen); Wouldn't it be better to declare outputlen (0002) as uint32? To avoid converting from (int) to (uint32), even if afterwards there is a conversion from (uint32) to (int)? regards, Ranier Vilela
Re: Multiple FPI_FOR_HINT for the same block during killing btree index items
Em sex., 15 de mai. de 2020 às 18:53, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Apr-10, Masahiko Sawada wrote: > > > Okay. I think only adding the check would also help with reducing the > > likelihood. How about the changes for the current HEAD I've attached? > > Pushed this to all branches. (Branches 12 and older obviously needed an > adjustment.) Thanks! > > > Related to this behavior on btree indexes, this can happen even on > > heaps during searching heap tuples. To reduce the likelihood of that > > more generally I wonder if we can acquire a lock on buffer descriptor > > right before XLogSaveBufferForHint() and set a flag to the buffer > > descriptor that indicates that we're about to log FPI for hint bit so > > that concurrent process can be aware of that. > > I'm not sure how that helps; the other process would have to go back and > redo their whole operation from scratch in order to find out whether > there's still something alive that needs killing. > > I think you need to acquire the exclusive lock sooner: if, when scanning > the page, you find a killable item, *then* upgrade the lock to exclusive > and restart the scan. This means that we'll have to wait for any other > process that's doing the scan, and they will all give up their share > lock to wait for the exclusive lock they need. So the one that gets it > first will do all the killing, log the page, then release the lock. At > that point the other processes will wake up and see that items have been > killed, so they will return having done nothing. > Regarding the block, I disagree in part, because in the worst case, the block can be requested in the last item analyzed, leading to redo all the work from the beginning. If we are in _bt_killitems it is because there is a high probability that there will be items to be deleted, why not request the block soon, if this meets the conditions? 1. XLogHintBitIsNeeded () 2.! AutoVacuumingActive () 3. New exclusive configuration variable option to activate the lock? Masahiko reported that it occurs only when (autovacuum_enabled = off); regards, Ranier Vilela avoid_killing_btree_items_already_dead_v2.patch Description: Binary data
Re: Multiple FPI_FOR_HINT for the same block during killing btree index items
Em sex., 15 de mai. de 2020 às 18:53, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Apr-10, Masahiko Sawada wrote: > > > Okay. I think only adding the check would also help with reducing the > > likelihood. How about the changes for the current HEAD I've attached? > > Pushed this to all branches. (Branches 12 and older obviously needed an > adjustment.) Thanks! > > > Related to this behavior on btree indexes, this can happen even on > > heaps during searching heap tuples. To reduce the likelihood of that > > more generally I wonder if we can acquire a lock on buffer descriptor > > right before XLogSaveBufferForHint() and set a flag to the buffer > > descriptor that indicates that we're about to log FPI for hint bit so > > that concurrent process can be aware of that. > > I'm not sure how that helps; the other process would have to go back and > redo their whole operation from scratch in order to find out whether > there's still something alive that needs killing. > > I think you need to acquire the exclusive lock sooner: if, when scanning > the page, you find a killable item, *then* upgrade the lock to exclusive > and restart the scan. This means that we'll have to wait for any other > process that's doing the scan, and they will all give up their share > lock to wait for the exclusive lock they need. So the one that gets it > first will do all the killing, log the page, then release the lock. At > that point the other processes will wake up and see that items have been > killed, so they will return having done nothing. > > Like the attached. I didn't verify that it works well or that it > actually improves performance ... > This is not related to your latest patch. But I believe I can improve the performance. So: 1. If killedsomething is false 2. Any killtuple is true and (not ItemIdIsDead(iid)) is false 3. Nothing to be done. So why do all the work and then discard it. We can eliminate the current item much earlier, testing if it is already dead. regards, Ranier VIlela avoid_killing_btree_items_aready_dead.patch Description: Binary data
Re: calling procedures is slow and consumes extra much memory against calling function
Em sáb., 16 de mai. de 2020 às 11:07, Pavel Stehule
escreveu:
>
>
> so 16. 5. 2020 v 15:24 odesílatel Ranier Vilela
> napsal:
>
>> Em sáb., 16 de mai. de 2020 às 09:35, Pavel Stehule <
>> pavel.steh...@gmail.com> escreveu:
>>
>>>
>>>
>>> so 16. 5. 2020 v 13:40 odesílatel Ranier Vilela
>>> napsal:
>>>
>>>> Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule <
>>>> pavel.steh...@gmail.com> escreveu:
>>>>
>>>>>
>>>>>
>>>>> so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela
>>>>> napsal:
>>>>>
>>>>>> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
>>>>>> pavel.steh...@gmail.com> escreveu:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela
>>>>>>> napsal:
>>>>>>>
>>>>>>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>>>>>>> pavel.steh...@gmail.com> escreveu:
>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> I try to use procedures in Orafce package, and I did some easy
>>>>>>>>> performance tests. I found some hard problems:
>>>>>>>>>
>>>>>>>>> 1. test case
>>>>>>>>>
>>>>>>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>>>>>>> begin v := random() * r; end
>>>>>>>>> $$ language plpgsql;
>>>>>>>>>
>>>>>>>>> This command requires
>>>>>>>>>
>>>>>>>>> do $$
>>>>>>>>> declare r int default 100; x int;
>>>>>>>>> begin
>>>>>>>>> for i in 1..30 loop
>>>>>>>>> call p1(r, x);
>>>>>>>>> end loop;
>>>>>>>>> end;
>>>>>>>>> $$;
>>>>>>>>>
>>>>>>>>> about 2.2GB RAM and 10 sec.
>>>>>>>>>
>>>>>>>> I am having a consistent result of 3 secs, with a modified version
>>>>>>>> (exec_stmt_call) of your patch.
>>>>>>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference
>>>>>>>> in the testing hardware?
>>>>>>>>
>>>>>>>
>>>>>>> My notebook is old T520, and more I have a configured Postgres with
>>>>>>> --enable-cassert option.
>>>>>>>
>>>>>> The hardware is definitely making a difference, but if you have time
>>>>>> and don't mind testing it,
>>>>>> I can send you a patch, not that the modifications are a big deal,
>>>>>> but maybe they'll help.
>>>>>>
>>>>> With more testing, I found that latency increases response time.
>>>> With 3 (secs) the test is with localhost.
>>>> With 6 (secs) the test is with tcp (local, not between pcs).
>>>>
>>>> Anyway, I would like to know if we have the number of parameters
>>>> previously, why use List instead of Arrays?
>>>> It would not be faster to create plpgsql variables.
>>>>
>>>
>>> Why you check SPI_processed?
>>>
>>> + if (SPI_processed == 1)
>>> + {
>>> + if (!stmt->target)
>>> + elog(ERROR, "DO statement returned a row, query \"%s\"", expr->query);
>>> + }
>>> + else if (SPI_processed > 1)
>>> + elog(ERROR, "Procedure call returned more than one row, query \"%s\"",
>>> expr->query);
>>>
>>>
>>> CALL cannot to return rows, so these checks has not sense
>>>
>> Looking at the original file, this already done, from line 2351,
>> I just put all the tests together to, if applicable, get out quickly.
>>
>
> It's little bit messy. Is not good to mix bugfix and refactoring things
> together
>
Ok, I can understand that.
regards,
Ranier Vilela
Re: calling procedures is slow and consumes extra much memory against calling function
Em sáb., 16 de mai. de 2020 às 09:35, Pavel Stehule
escreveu:
>
>
> so 16. 5. 2020 v 13:40 odesílatel Ranier Vilela
> napsal:
>
>> Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule <
>> pavel.steh...@gmail.com> escreveu:
>>
>>>
>>>
>>> so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela
>>> napsal:
>>>
>>>> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
>>>> pavel.steh...@gmail.com> escreveu:
>>>>
>>>>>
>>>>>
>>>>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela
>>>>> napsal:
>>>>>
>>>>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>>>>> pavel.steh...@gmail.com> escreveu:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> I try to use procedures in Orafce package, and I did some easy
>>>>>>> performance tests. I found some hard problems:
>>>>>>>
>>>>>>> 1. test case
>>>>>>>
>>>>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>>>>> begin v := random() * r; end
>>>>>>> $$ language plpgsql;
>>>>>>>
>>>>>>> This command requires
>>>>>>>
>>>>>>> do $$
>>>>>>> declare r int default 100; x int;
>>>>>>> begin
>>>>>>> for i in 1..30 loop
>>>>>>> call p1(r, x);
>>>>>>> end loop;
>>>>>>> end;
>>>>>>> $$;
>>>>>>>
>>>>>>> about 2.2GB RAM and 10 sec.
>>>>>>>
>>>>>> I am having a consistent result of 3 secs, with a modified version
>>>>>> (exec_stmt_call) of your patch.
>>>>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference in
>>>>>> the testing hardware?
>>>>>>
>>>>>
>>>>> My notebook is old T520, and more I have a configured Postgres with
>>>>> --enable-cassert option.
>>>>>
>>>> The hardware is definitely making a difference, but if you have time
>>>> and don't mind testing it,
>>>> I can send you a patch, not that the modifications are a big deal, but
>>>> maybe they'll help.
>>>>
>>> With more testing, I found that latency increases response time.
>> With 3 (secs) the test is with localhost.
>> With 6 (secs) the test is with tcp (local, not between pcs).
>>
>> Anyway, I would like to know if we have the number of parameters
>> previously, why use List instead of Arrays?
>> It would not be faster to create plpgsql variables.
>>
>
> Why you check SPI_processed?
>
> + if (SPI_processed == 1)
> + {
> + if (!stmt->target)
> + elog(ERROR, "DO statement returned a row, query \"%s\"", expr->query);
> + }
> + else if (SPI_processed > 1)
> + elog(ERROR, "Procedure call returned more than one row, query \"%s\"",
> expr->query);
>
>
> CALL cannot to return rows, so these checks has not sense
>
Looking at the original file, this already done, from line 2351,
I just put all the tests together to, if applicable, get out quickly.
regards,
Ranier Vilela
Re: calling procedures is slow and consumes extra much memory against calling function
Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule escreveu: > > > so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela > napsal: > >> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule < >> pavel.steh...@gmail.com> escreveu: >> >>> >>> >>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela >>> napsal: >>> >>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule < >>>> pavel.steh...@gmail.com> escreveu: >>>> >>>>> Hi >>>>> >>>>> I try to use procedures in Orafce package, and I did some easy >>>>> performance tests. I found some hard problems: >>>>> >>>>> 1. test case >>>>> >>>>> create or replace procedure p1(inout r int, inout v int) as $$ >>>>> begin v := random() * r; end >>>>> $$ language plpgsql; >>>>> >>>>> This command requires >>>>> >>>>> do $$ >>>>> declare r int default 100; x int; >>>>> begin >>>>> for i in 1..30 loop >>>>> call p1(r, x); >>>>> end loop; >>>>> end; >>>>> $$; >>>>> >>>>> about 2.2GB RAM and 10 sec. >>>>> >>>> I am having a consistent result of 3 secs, with a modified version >>>> (exec_stmt_call) of your patch. >>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference in >>>> the testing hardware? >>>> >>> >>> My notebook is old T520, and more I have a configured Postgres with >>> --enable-cassert option. >>> >> The hardware is definitely making a difference, but if you have time and >> don't mind testing it, >> I can send you a patch, not that the modifications are a big deal, but >> maybe they'll help. >> > With more testing, I found that latency increases response time. With 3 (secs) the test is with localhost. With 6 (secs) the test is with tcp (local, not between pcs). Anyway, I would like to know if we have the number of parameters previously, why use List instead of Arrays? It would not be faster to create plpgsql variables. regards, Ranier Vilela pl_exec_test.patch Description: Binary data
Re: calling procedures is slow and consumes extra much memory against calling function
Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule escreveu: > > > so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela > napsal: > >> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule < >> pavel.steh...@gmail.com> escreveu: >> >>> Hi >>> >>> I try to use procedures in Orafce package, and I did some easy >>> performance tests. I found some hard problems: >>> >>> 1. test case >>> >>> create or replace procedure p1(inout r int, inout v int) as $$ >>> begin v := random() * r; end >>> $$ language plpgsql; >>> >>> This command requires >>> >>> do $$ >>> declare r int default 100; x int; >>> begin >>> for i in 1..30 loop >>> call p1(r, x); >>> end loop; >>> end; >>> $$; >>> >>> about 2.2GB RAM and 10 sec. >>> >> I am having a consistent result of 3 secs, with a modified version >> (exec_stmt_call) of your patch. >> But my notebook is (Core 5, 8GB and SSD), could it be a difference in the >> testing hardware? >> > > My notebook is old T520, and more I have a configured Postgres with > --enable-cassert option. > The hardware is definitely making a difference, but if you have time and don't mind testing it, I can send you a patch, not that the modifications are a big deal, but maybe they'll help. regards, Ranier Vilela
Re: calling procedures is slow and consumes extra much memory against calling function
Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule escreveu: > Hi > > I try to use procedures in Orafce package, and I did some easy performance > tests. I found some hard problems: > > 1. test case > > create or replace procedure p1(inout r int, inout v int) as $$ > begin v := random() * r; end > $$ language plpgsql; > > This command requires > > do $$ > declare r int default 100; x int; > begin > for i in 1..30 loop > call p1(r, x); > end loop; > end; > $$; > > about 2.2GB RAM and 10 sec. > I am having a consistent result of 3 secs, with a modified version (exec_stmt_call) of your patch. But my notebook is (Core 5, 8GB and SSD), could it be a difference in the testing hardware? regards, Ranier Vilela
Re: [PATCH] Fix ouside scope t_ctid (ItemPointerData)
Em qui., 14 de mai. de 2020 às 19:49, Mark Dilger <
mark.dil...@enterprisedb.com> escreveu:
>
>
> > On May 14, 2020, at 11:34 AM, Ranier Vilela wrote:
> >
> > Certainly.
> > In the same file you can find the appropriate use of the API.
> > ItemPointerSet(&heapTuple->t_self, blkno, offnum);
>
> It took a couple reads through your patch to figure out what you were
> trying to accomplish, and I think you are uncomfortable with assigning one
> ItemPointerData variable from another.ItemPointerData is just a struct
> with three int16 variables. To make a standalone program that has the same
> structure without depending on any postgres headers, I'm using "short int"
> instead of "int16" and structs "TwoData" and "ThreeData" that are analogous
> to BlockIdData and OffsetNumber.
>
> #include
>
> typedef struct TwoData {
> short int a;
> short int b;
> } TwoData;
>
> typedef struct ThreeData {
> TwoData left;
> short int right;
> } ThreeData;
>
> int main(int argc, char **argv)
> {
> ThreeData x = { { 5, 10 }, 15 };
> ThreeData y = x;
> x.left.a = 0;
> x.left.b = 1;
> x.right = 2;
>
> printf("y = { { %d, %d }, %d }\n",
> y.left.a, y.left.b, y.right);
>
> return 0;
> }
>
> If you compile and run this, you'll notice it outputs:
>
> y = { { 5, 10 }, 15 }
>
> and not the { { 0, 1}, 2 } that you would expect if y were merely pointing
> at x.
>
Thanks for the example.
But what I wanted to test was
struct1 = struct2;
Both being of the same type of structure.
What I wrongly deduced was that the address of struct2 was saved and not
its content.
Again, thanks for your time and clarification.
regards,
Ranier Vilela
Re: [PATCH] Fix ouside scope t_ctid (ItemPointerData)
Em qui., 14 de mai. de 2020 às 19:23, Mark Dilger < mark.dil...@enterprisedb.com> escreveu: > > > > On May 14, 2020, at 11:34 AM, Ranier Vilela wrote: > > > > htup->t_ctid = target_tid; > > htup->t_ctid = newtid; > > Both target_tid and newtid are local variable, whe loss scope, memory is > garbage. > > Ok, thanks for the concrete example of what is bothering you. > > In htup_details, I see that struct HeapTupleHeaderData has a field named > t_ctid of type struct ItemPointerData. I also see in heapam that > target_tid is of type ItemPointerData. The > > htup->t_ctid = target_tid > > copies the contents of target_tid. By the time target_tid goes out of > scope, the contents are already copied. I would share your concern if > t_ctid were of type ItemPointer (aka ItemPointerData *) and the code said > > htup->t_ctid = &target_tid > > but it doesn't say that, so I don't see the issue. > Even if the patch simplifies and uses the API to make the assignments. Really, the central problem does not exist, my fault. Perhaps because it has never made such use, structure assignment. And I failed to do research on the subject before. Sorry. > > Also in heapam, I see that newtid is likewise of type ItemPointerData, so > the same logic applies. By the time newtid goes out of scope, its contents > have already been copied into t_ctid, so there is no problem. > > But maybe you know all that and are just complaining that the name > "ItemPointerData" sounds like a pointer rather than a struct? I'm still > unclear whether you believe this is a bug, or whether you just don't like > the naming that is used. > My concerns were about whether attribution really would copy the structure's content and not just its address. The name makes it difficult, but that was not the point. The tool warned about uninitialized variable, which I mistakenly deduced for loss of scope. Thank you very much for the clarifications and your time. We never stopped learning, and using structure assignment was a new learning experience. regards Ranier Vilela
Re: [PATCH] Fix ouside scope t_ctid (ItemPointerData)
Em qui., 14 de mai. de 2020 às 15:07, Tom Lane escreveu: > Ranier Vilela writes: > > The patch is primarily intended to correct the use of ItemPointerData. > > What do you think is being "corrected" here? It looks to me like > just some random code rearrangements that aren't even clearly > bug-free, let alone being stylistic improvements. > It is certainly working, but trusting that the memory of a local variable will not change, when it loses its scope, is a risk that, certainly, can cause bugs, elsewhere. And it is certainly very difficult to discover its primary cause. regards, Ranier Vilela
Re: [PATCH] Fix ouside scope t_ctid (ItemPointerData)
Em qui., 14 de mai. de 2020 às 15:03, Mark Dilger <
mark.dil...@enterprisedb.com> escreveu:
>
>
> > On May 14, 2020, at 10:40 AM, Ranier Vilela wrote:
> >
> > Hi,
> > ItemPointerData, on the contrary, from what the name says,
> > it is not a pointer to a structure, but a structure in fact.
>
> The project frequently uses the pattern
>
> typedef struct FooData {
> ...
> } FooData;
>
> typedef FooData *Foo;
>
> where, in this example, "Foo" = "ItemPointer".
>
> So the "Data" part of "ItemPointerData" clues the reader into the fact
> that ItemPointerData is a struct, not a pointer. Granted, the "Pointer"
> part of that name may confuse some readers, but the struct itself does
> contain what is essentially a 48-bit pointer, so that name is not nuts.
>
>
> > When assigning the name of the structure variable to a pointer, it may
> even work,
> > but, it is not the right thing to do and it becomes a nightmare,
> > to discover that any other error they have is at cause.
>
> Can you give a code example of the type of assigment you mean?
>
htup->t_ctid = target_tid;
htup->t_ctid = newtid;
Both target_tid and newtid are local variable, whe loss scope, memory is
garbage.
>
> > So:
> > 1. In some cases, there may be a misunderstanding in the use of
> ItemPointerData.
> > 2. When using the variable name in an assignment, the variable's address
> is used.
> > 3. While this works for a structure, it shouldn't be the right thing to
> do.
> > 4. If we have a local variable, its scope is limited and when it loses
> its scope, memory is certainly garbage.
> > 5. While this may be working for heapam.c, I believe it is being abused
> and should be compliant with
> > the Postgres API and use the functions that were created for this.
> >
> > The patch is primarily intended to correct the use of ItemPointerData.
> > But it is also changing the style, reducing the scope of some variables.
> > If that was not acceptable, reduce the scope and someone has objections,
> > I can change the patch, to focus only on the use of ItemPointerData.
> > But as style changes are rare, if possible, it would be good to seize
> the opportunity.
>
> I would like to see a version of the patch that only addresses your
> concerns about ItemPointerData, not because other aspects of the patch are
> unacceptable (I'm not ready to even contemplate that yet), but because I'm
> not sure what your complaint is about. Can you restrict the patch to just
> address that one issue?
>
Certainly.
In the same file you can find the appropriate use of the API.
ItemPointerSet(&heapTuple->t_self, blkno, offnum);
regards,
Ranier Vilela
001_fix_outside_scope_t_cid_v2.patch
Description: Binary data
[PATCH] Fix ouside scope t_ctid (ItemPointerData)
Hi, ItemPointerData, on the contrary, from what the name says, it is not a pointer to a structure, but a structure in fact. When assigning the name of the structure variable to a pointer, it may even work, but, it is not the right thing to do and it becomes a nightmare, to discover that any other error they have is at cause. So: 1. In some cases, there may be a misunderstanding in the use of ItemPointerData. 2. When using the variable name in an assignment, the variable's address is used. 3. While this works for a structure, it shouldn't be the right thing to do. 4. If we have a local variable, its scope is limited and when it loses its scope, memory is certainly garbage. 5. While this may be working for heapam.c, I believe it is being abused and should be compliant with the Postgres API and use the functions that were created for this. The patch is primarily intended to correct the use of ItemPointerData. But it is also changing the style, reducing the scope of some variables. If that was not acceptable, reduce the scope and someone has objections, I can change the patch, to focus only on the use of ItemPointerData. But as style changes are rare, if possible, it would be good to seize the opportunity. regards, Ranier Vilela 001_fix_outside_scope_t_ctid.patch Description: Binary data
Re: PG compilation error with Visual Studio 2015/2017/2019
Em qui., 14 de mai. de 2020 às 05:49, Amit Kapila escreveu: > On Wed, May 13, 2020 at 7:34 PM Tom Lane wrote: > > > > Amit Kapila writes: > > > Now that branches are tagged, I would like to commit and backpatch > > > this patch tomorrow unless there are any more comments/objections. > > > > The "quiet period" is over as soon as the tags appear in git. > > > > Pushed. > Thank you for the commit. regards, Ranier Vilela
Re: SLRU statistics
Em qua., 13 de mai. de 2020 às 11:46, Fujii Masao < masao.fu...@oss.nttdata.com> escreveu: > > > On 2020/05/13 23:26, Tom Lane wrote: > > Fujii Masao writes: > >> On 2020/05/13 17:21, Tomas Vondra wrote: > >>> On Wed, May 13, 2020 at 04:10:30PM +0900, Fujii Masao wrote: > >>>> Also I found another minor issue; SLRUStats has not been initialized > to 0 > >>>> and which could update the counters unexpectedly. Attached patch fixes > >>>> this issue. > > > >> Pushed both. Thanks! > > > > Why is that necessary? A static variable is defined by C to start off > > as zeroes. > > Because SLRUStats is not a static variable. No? > IMHO, BgWriterStats have the same problem, shouldn't the same be done? /* Initialize BgWriterStats to zero */ MemSet(&BgWriterStats, 0, sizeof(BgWriterStats)); /* Initialize SLRU statistics to zero */ memset(&SLRUStats, 0, sizeof(SLRUStats)); regards, Ranier Vilela
Re: [PATCH] Fix division by zero (explain.c)
Em sáb., 9 de mai. de 2020 às 17:48, Tomas Vondra <
tomas.von...@2ndquadrant.com> escreveu:
> On Sat, May 09, 2020 at 02:51:50PM -0300, Ranier Vilela wrote:
> >Em sáb., 9 de mai. de 2020 às 14:44, Tomas Vondra <
> >tomas.von...@2ndquadrant.com> escreveu:
> >
> >> On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
> >> >Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane
> >> escreveu:
> >> >
> >> >> James Coleman writes:
> >> >> > There are always full sort groups before any prefix groups can
> happen,
> >> >> > so we know (even though the tooling doesn't) that the 2nd test can
> >> >> > never contradict the first.
> >> >>
> >> >> So maybe an assertion enforcing that would be appropriate?
> >> >> Untested, but:
> >> >>
> >> >> - if (fullsortGroupInfo->groupCount == 0 &&
> >> >> - prefixsortGroupInfo->groupCount == 0)
> >> >> + if (fullsortGroupInfo->groupCount == 0)
> >> >> + {
> >> >> +
> Assert(prefixsortGroupInfo->groupCount
> >> ==
> >> >> 0);
> >> >> continue;
> >> >> + }
> >> >>
> >> >I agree, asserts always help.
> >> >
> >>
> >> That doesn't work, because the prefixSortGroupInfo is used before
> >> assignment, producing compile-time warnings.
> >>
> >> I've pushed a simpler fix without the assert. If we want to make this
> >> check, perhaps doing it in incremental sort itself would be better than
> >> doing it in explain.
> >>
> >Thanks anyway for the commit.
> >But if you used the first version of my patch, would the author be me and
> >am I as reported?
> >What does it take to be considered the author?
> >
>
> Apologies. I should have listed you as an author, not just in the
> reported-by field.
>
Apologies accepted.
Thank you.
Ranier VIilela
Re: [PATCH] Fix division by zero (explain.c)
Em sáb., 9 de mai. de 2020 às 14:44, Tomas Vondra <
tomas.von...@2ndquadrant.com> escreveu:
> On Sat, May 09, 2020 at 06:48:59AM -0300, Ranier Vilela wrote:
> >Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane
> escreveu:
> >
> >> James Coleman writes:
> >> > There are always full sort groups before any prefix groups can happen,
> >> > so we know (even though the tooling doesn't) that the 2nd test can
> >> > never contradict the first.
> >>
> >> So maybe an assertion enforcing that would be appropriate?
> >> Untested, but:
> >>
> >> - if (fullsortGroupInfo->groupCount == 0 &&
> >> - prefixsortGroupInfo->groupCount == 0)
> >> + if (fullsortGroupInfo->groupCount == 0)
> >> + {
> >> + Assert(prefixsortGroupInfo->groupCount
> ==
> >> 0);
> >> continue;
> >> + }
> >>
> >I agree, asserts always help.
> >
>
> That doesn't work, because the prefixSortGroupInfo is used before
> assignment, producing compile-time warnings.
>
> I've pushed a simpler fix without the assert. If we want to make this
> check, perhaps doing it in incremental sort itself would be better than
> doing it in explain.
>
Thanks anyway for the commit.
But if you used the first version of my patch, would the author be me and
am I as reported?
What does it take to be considered the author?
regards,
Ranier Vilela
Re: [PATCH] Fix division by zero (explain.c)
Em sáb., 9 de mai. de 2020 às 01:45, Tom Lane escreveu:
> James Coleman writes:
> > There are always full sort groups before any prefix groups can happen,
> > so we know (even though the tooling doesn't) that the 2nd test can
> > never contradict the first.
>
> So maybe an assertion enforcing that would be appropriate?
> Untested, but:
>
> - if (fullsortGroupInfo->groupCount == 0 &&
> - prefixsortGroupInfo->groupCount == 0)
> + if (fullsortGroupInfo->groupCount == 0)
> + {
> + Assert(prefixsortGroupInfo->groupCount ==
> 0);
> continue;
> + }
>
I agree, asserts always help.
regards,
Ranier Vilela
fix_division_by_zero_explain_v2.patch
Description: Binary data
Re: [PATCH] Fix division by zero (explain.c)
Em sex., 8 de mai. de 2020 às 19:02, Tomas Vondra < tomas.von...@2ndquadrant.com> escreveu: > On Thu, Apr 23, 2020 at 04:12:34PM -0400, James Coleman wrote: > >On Thu, Apr 23, 2020 at 8:38 AM Ranier Vilela > wrote: > >> > >> Hi, > >> > >> Per Coverity. > >> > >> If has 0 full groups, "we don't need to do anything" and need goes to > next. > >> Otherwise a integer division by zero, can raise. > >> > >> comments extracted trom explain.c: > >> /* > >> * Since we never have any prefix groups unless we've first sorted > >> * a full groups and transitioned modes (copying the tuples into a > >> * prefix group), we don't need to do anything if there were 0 full > >> * groups. > >> */ > > > >This does look like a fairly obvious thinko on my part, and the patch > >looks correct to me. > > > >Tomas: agreed? > > > > So how do we actually get the division by zero? It seems to me the fix > prevents a division by zero with 0 full groups and >0 prefix groups, > but can that actually happen? > > But can that actually happen? Doesn't the comment quoted in the report > actually suggest otherwise? If this > >(fullsortGroupInfo->groupCount == 0 && > prefixsortGroupInfo->groupCount == 0) > > First this line, contradicts the comments. According to the comments, if ( fullsortGroupInfo->groupCount == 0) is true, there is no need to do anything else, next. So anyway, we don't need to test anything anymore. Now, to happen the division by zero, (prefixsortGroupInfo->groupCount == 0, needs to be true too, Maybe this is not happening, but if it happens, it divides by zero, just below, so if an unnecessary test and adds a risk, why not, remove it? > evaluates to false, and > >(fullsortGroupInfo->groupCount == 0) > > this evaluates to true, then clearly there would have to be 0 full > groups and >0 prefix groups. But the comment says that can't happen, > unless I misunderstand what it's saying. > Comments says: "we don't need to do anything if there were 0 full groups." regards, Ranier Vilela
Re: pg_restore error message
Em qui., 7 de mai. de 2020 às 18:54, Euler Taveira < euler.tave...@2ndquadrant.com> escreveu: > Hi, > > While investigating a pg_restore error, I stumbled upon a message that is > not so useful. > > pg_restore: error: could not close data file: No such file or directory > > Which file? File name should be printed too like in the error check for > cfopen_read a few lines above. > Can suggest improvements? 1. free (398 line) must be pg_free(buf)'; 2. %m, is a format to parameter, right? But what parameter? Both fatal call, do not pass this parameter, or is it implied? regards, Ranier Vilela
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qui., 7 de mai. de 2020 às 02:04, Victor Wagner
escreveu:
> В Thu, 7 May 2020 09:14:33 +0900
> Michael Paquier пишет:
>
> > On Wed, May 06, 2020 at 03:58:15PM -0300, Ranier Vilela wrote:
> > > Hacking pgbison.pl, to print PATH, shows that the path inside
> > > pgbison.pl, returned to being the original, without the addition of
> > > c:\perl\bin;c:\bin. my $out = $ENV{PATH};
> > > print "Path after system call=$out\n";
> > > Path after system
> > > call=...C:\Users\ranier\AppData\Local\Microsoft\WindowsApps;;
> > > The final part lacks: c:\perl\bin;c:\bin
> > >
> > > Now I need to find out why the path is being reset, within the perl
> > > scripts.
> >
> > FWIW, we have a buildfarm animal called drongo that runs with VS 2019,
> > that uses Python, and that is now happy. One of my own machines uses
> > VS 2019 as well and I have yet to see what you are describing here.
> > Perhaps that's related to a difference in the version of perl you are
> > using and the version of that any others?
>
>
> I doubt so. I have different machines with perl from 5.22 to 5.30 but
> none of tham exibits such weird behavoir.
>
The perl is the same,when it was working ok.
>
> Perhaps problem is that Ranier calls vcvars64.bat from the menu, and
> then calls msbuild such way that is becames unrelated process.
>
It also worked previously, using this same process, link menu and manual
path configuration.
What has changed:
1 In the environment, the python installation, which added entries to the
path.
2. Perl scripts: Use perl's $/ more idiomatically
commit beb2516e961490723fb1a2f193406afb3d71ea9c
3. Msbuild and others, have been updated.They are not the same ones that
were working before.
>
> Obvoisly buildfarm animal doesn't use menu and then starts build
> process from same CMD.EXE process, that it called vcvarsall.but into.
>
> It is same in all OSes - Windows, *nix and even MS-DOS - there is no
> way to change environment of parent process. You can change environment
> of current process (and if this process is command interpreter you can
> do so by sourcing script into it. In windows this misleadingly called
> 'CALL', but it executes commands from command file in the current
> shell, not in subshell) you can pass enivronment to the child
> processes. But you can never affect environment of the parent or
> sibling process.
>
Maybe that's what is happening, calling system, perl or msbuild, would be
creating a new environment, transferring the path that is configured in
Windows, and not the path that is in the environment that was manually
configured.
>
> The only exception is - if you know that some process would at startup
> read environment vars from some file or registry, you can modify this
> source in unrelated process.
>
> So, if you want perl in path of msbuild, started from Visual Studio,
> you should either first set path in CMD.EXE, then type command to start
> Studio from this very command window, or set path via control panel
> dialog (which modified registry). Later is what I usially do on machines
> wher I compile postgres.
>
buidfarm aninal, uses a more secure and reliable process, the path is
already configured and does not change.
Perhaps this is the way for me and for others.
It would then remain to document, to warn that to work correctly, the path
must be configured before entering the compilation environment.
regards,
Ranier Vilela
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qua., 6 de mai. de 2020 às 21:14, Michael Paquier
escreveu:
> On Wed, May 06, 2020 at 03:58:15PM -0300, Ranier Vilela wrote:
> > Hacking pgbison.pl, to print PATH, shows that the path inside pgbison.pl
> ,
> > returned to being the original, without the addition of
> c:\perl\bin;c:\bin.
> > my $out = $ENV{PATH};
> > print "Path after system call=$out\n";
> > Path after system
> > call=...C:\Users\ranier\AppData\Local\Microsoft\WindowsApps;;
> > The final part lacks: c:\perl\bin;c:\bin
> >
> > Now I need to find out why the path is being reset, within the perl
> scripts.
>
> FWIW, we have a buildfarm animal called drongo that runs with VS 2019,
> that uses Python, and that is now happy. One of my own machines uses
> VS 2019 as well and I have yet to see what you are describing here.
> Perhaps that's related to a difference in the version of perl you are
> using and the version of that any others?
>
I really don't know what to say, I know very little about perl.
The perl is:
Win32 strawberry-perl 5.30.1.1
regards,
Ranier VIlela
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qua., 6 de mai. de 2020 às 21:08, Michael Paquier escreveu: > On Wed, May 06, 2020 at 02:11:34PM -0400, Andrew Dunstan wrote: > > We assume perl, flex and bison are in the PATH. That doesn't seem > > unreasonable, it's worked well for quite a long time. > > I recall that it is an assumption we rely on since MSVC scripts are > around, and that's rather easy to configure, so it seems to me that > changing things now would just introduce annoying changes for anybody > (developers, maintainers) using this stuff. > Ah yes, better to leave it as is. No problem for me, I already got around the difficulty. regards, Ranier Vilela
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qua., 6 de mai. de 2020 às 15:19, Ranier Vilela escreveu: > Em qua., 6 de mai. de 2020 às 14:14, Victor Wagner > escreveu: > >> В Wed, 6 May 2020 10:21:41 -0300 >> Ranier Vilela пишет: >> >> > Em qua., 6 de mai. de 2020 às 09:53, Michael Paquier >> > escreveu: >> > >> > > On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote: >> > > > I agree, it is better. >> > > >> > > Thanks, applied and back-patched down to 9.5. Now for the second >> > > problem of this thread.. >> > > >> > Sorry, no clue yet. >> > I hacked the perl sources, to hardcoded perl, bison and flex with >> > path.It works like this. >> >> Perl has "magic" variable $^X which expands to full path to perl >> executable, I wonder why build.pl doesn't use it to invoke secondary >> perl scripts. >> > I still don't think it's necessary, it was working well. > My main suspicions are: > 1. Path with spaces; > 2. Incompatibility with < symbol, some suggest use " > >
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qua., 6 de mai. de 2020 às 14:14, Victor Wagner escreveu: > В Wed, 6 May 2020 10:21:41 -0300 > Ranier Vilela пишет: > > > Em qua., 6 de mai. de 2020 às 09:53, Michael Paquier > > escreveu: > > > > > On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote: > > > > I agree, it is better. > > > > > > Thanks, applied and back-patched down to 9.5. Now for the second > > > problem of this thread.. > > > > > Sorry, no clue yet. > > I hacked the perl sources, to hardcoded perl, bison and flex with > > path.It works like this. > > Perl has "magic" variable $^X which expands to full path to perl > executable, I wonder why build.pl doesn't use it to invoke secondary > perl scripts. > I still don't think it's necessary, it was working well. My main suspicions are: 1. Path with spaces; 2. Incompatibility with < symbol, some suggest use "
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qua., 6 de mai. de 2020 às 10:25, Ranier Vilela
escreveu:
> Em qua., 6 de mai. de 2020 às 10:21, Ranier Vilela
> escreveu:
>
>> Em qua., 6 de mai. de 2020 às 09:53, Michael Paquier
>> escreveu:
>>
>>> On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote:
>>> > I agree, it is better.
>>>
>>> Thanks, applied and back-patched down to 9.5. Now for the second
>>> problem of this thread..
>>>
>> Sorry, no clue yet.
>> I hacked the perl sources, to hardcoded perl, bison and flex with path.It
>> works like this.
>> For some reason, which I haven't yet discovered, msbuild is ignoring the
>> path, where perl and bison and flex are.
>> Although it is being set, within the 64-bit compilation environment of
>> msvc 2019.
>> I'm still investigating.
>>
> In fact perl, it is found, otherwise, neither build.pl would be working.
> But within the perl environment, when the system call is made, in this
> case, neither perl, bison, nor flex is found.
>
I'm using it like this, for now.
File pgbison.pl:
system("c:\\bin\\bison $headerflag $input -o $output");
File pgflex.pl:
system("c:\\bin\\flex $flexflags -o$output $input");
system("c:\\perl\\bin\\perl src\\tools\\fix-old-flex-code.pl
$output");
File Solution.pm:
system(
system('perl generate-lwlocknames.pl lwlocknames.txt');
system(
system(
system(
system(
system(
system(
system("perl create_help.pl ../../../doc/src/sgml/ref
sql_help");
system(
system(
system(
system(
system(
system('perl parse.pl < ../../../backend/parser/gram.y >
preproc.y');
system(
C:\dll\postgres\src\tools\msvc>\bin\grep bison *pm
File MSBuildProject.pm:
Running
bison on $grammarFile
c:\\perl\\bin\\perl
"src\\tools\\msvc\\pgbison.pl" "$grammarFile"
Running
bison on $grammarFile
c:\\perl\\bin\\perl
"src\\tools\\msvc\\pgbison.pl" "$grammarFile"
C:\dll\postgres\src\tools\msvc>\bin\grep flex *pm
File MSBuildProject.pm:
Running
flex on $grammarFile
c:\\perl\\bin\\perl
"src\\tools\\msvc\\pgflex.pl" "$grammarFile"
Running
flex on $grammarFile
c:\\perl\\bin\\perl
"src\\tools\\msvc\\pgflex.pl" "$grammarFile"
regards,
Ranier Vilela
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qua., 6 de mai. de 2020 às 10:21, Ranier Vilela escreveu: > Em qua., 6 de mai. de 2020 às 09:53, Michael Paquier > escreveu: > >> On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote: >> > I agree, it is better. >> >> Thanks, applied and back-patched down to 9.5. Now for the second >> problem of this thread.. >> > Sorry, no clue yet. > I hacked the perl sources, to hardcoded perl, bison and flex with path.It > works like this. > For some reason, which I haven't yet discovered, msbuild is ignoring the > path, where perl and bison and flex are. > Although it is being set, within the 64-bit compilation environment of > msvc 2019. > I'm still investigating. > In fact perl, it is found, otherwise, neither build.pl would be working. But within the perl environment, when the system call is made, in this case, neither perl, bison, nor flex is found. regards, Ranier Vilela
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qua., 6 de mai. de 2020 às 09:53, Michael Paquier escreveu: > On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote: > > I agree, it is better. > > Thanks, applied and back-patched down to 9.5. Now for the second > problem of this thread.. > Sorry, no clue yet. I hacked the perl sources, to hardcoded perl, bison and flex with path.It works like this. For some reason, which I haven't yet discovered, msbuild is ignoring the path, where perl and bison and flex are. Although it is being set, within the 64-bit compilation environment of msvc 2019. I'm still investigating. regards, Ranier Vilela
Re: Unify drop-by-OID functions
Only as a homework, is this function completely useless?
ItemPointerData
systable_scan_next(Relation heapRelation,
Oid indexId,
bool indexOK,
Snapshot snapshot,
int nkeys, ScanKey key)
{
SysScanDesc scan;
HeapTuple htup;
ItemPointerData tid;
scan = systable_beginscan(heapRelation, indexId, indexOK, snapshot,
nkeys, key);
htup = systable_getnext(scan);
if (HeapTupleIsValid(htup))
tid = &htup->t_self;
else
tid = NULL;
systable_endscan(scan);
return tid;
}
regards,
Ranier Vilela
Re: Unify drop-by-OID functions
Em ter., 5 de mai. de 2020 às 18:11, Alvaro Herrera <
alvhe...@2ndquadrant.com> escreveu:
> On 2020-May-05, Ranier Vilela wrote:
>
> > And in that specific case, leaving resources blocked, which perhaps, in
> my
> > humble opinion, could be released quickly.
>
> I very much doubt that you can measure any difference at all between
> these two codings of the function.
>
> I agree with the principle you mention, of not holding resources for
> long if they can be released earlier; but in this case the table_close
> call occurs across a ReleaseSysCache() call, which is hardly of
> significance. It's not like you have to wait for some other
> transaction, or wait for I/O, or anything like that that would make it
> measurable.
>
Locally it may not be as efficient, but it is a question, to follow the
good programming practices, among them, not to break anything, not to
block, and if it is not possible, release soon.
In at least one case, there will not even be a block, which is an
improvement.
Another version, that could, be, without using ERROR.
+static void
+DropObjectById(const ObjectAddress *object)
+{
+ int cacheId;
+ Relation rel;
+ HeapTuple tup;
+
+ cacheId = get_object_catcache_oid(object->classId);
+
+ /*
+ * Use the system cache for the oid column, if one exists.
+ */
+ if (cacheId >= 0)
+ {
+ tup = SearchSysCache1(cacheId, ObjectIdGetDatum(object->objectId));
+ if (HeapTupleIsValid(tup)) {
+rel = table_open(object->classId, RowExclusiveLock);
+CatalogTupleDelete(rel, &tup->t_self);
+table_close(rel, RowExclusiveLock);
+ReleaseSysCache(tup);
+ }
+ else
+ elog(LOG, "cache lookup failed for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+ }
+ else
+ {
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+
+ ScanKeyInit(&skey[0],
+ get_object_attnum_oid(object->classId),
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(object->objectId));
+
+rel = table_open(object->classId, RowExclusiveLock);
+ scan = systable_beginscan(rel, get_object_oid_index(object->classId),
true,
+ NULL, 1, skey);
+
+ /* we expect exactly one match */
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+CatalogTupleDelete(rel, &tup->t_self);
+ else
+ elog(LOG, "could not find tuple for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+systable_endscan(scan);
+table_close(rel, RowExclusiveLock);
+ }
+}
+
regards,
Ranier Vilela
Re: Unify drop-by-OID functions
Em ter., 5 de mai. de 2020 às 14:57, Robert Haas
escreveu:
> On Tue, May 5, 2020 at 1:43 PM Ranier Vilela wrote:
> > I see, the famous "cliché".
>
> By using the word cliché, and by putting it in quotes, you seem to
> suggest that you consider my argument dubious. However, I stand by it.
> Code shouldn't be changed without understanding the reasons behind the
> current coding. Doing so very often breaks things.
>
Sorry Robert, It was not my intention. I didn't know that using quotes
would change your understanding.
Of course, I find your arguments very valid and valuable.
And I understand that there are many interrelated things, which can break
if done in the wrong order.
Maybe I used the wrong word, in this case, the cliché.
What I mean is, the cliché, does some strange things, like leaving
variables to be declared, assigned in and not used.
And in that specific case, leaving resources blocked, which perhaps, in my
humble opinion, could be released quickly.
I think the expected behavior is being the same, with the changes I
proposed, IMHO.
+static void
+DropObjectById(const ObjectAddress *object)
+{
+ int cacheId;
+ Relation rel;
+ HeapTuple tup;
+
+ cacheId = get_object_catcache_oid(object->classId);
+
+ /*
+ * Use the system cache for the oid column, if one exists.
+ */
+ if (cacheId >= 0)
+ {
+ tup = SearchSysCache1(cacheId, ObjectIdGetDatum(object->objectId));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+rel = table_open(object->classId, RowExclusiveLock);
+ CatalogTupleDelete(rel, &tup->t_self);
+table_close(rel, RowExclusiveLock);
+
+ ReleaseSysCache(tup);
+ }
+ else
+ {
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+
+ ScanKeyInit(&skey[0],
+ get_object_attnum_oid(object->classId),
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(object->objectId));
+
+rel = table_open(object->classId, RowExclusiveLock);
+ scan = systable_beginscan(rel, get_object_oid_index(object->classId),
true,
+ NULL, 1, skey);
+
+ /* we expect exactly one match */
+ tup = systable_getnext(scan);
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "could not find tuple for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+ CatalogTupleDelete(rel, &tup->t_self);
+ systable_endscan(scan);
+
+table_close(rel, RowExclusiveLock);
+ }
+}
And again, your opinion is very important to me.
best regards,
Ranier Vilela
Re: Unify drop-by-OID functions
Em ter., 5 de mai. de 2020 às 14:29, Robert Haas escreveu: > On Tue, May 5, 2020 at 1:22 PM Ranier Vilela wrote: > > Ok, so the question. If (3) is not safe, obvious we shouldn't use, and > must call table_close, after systable_endscan. > > Now (1) and (2), I would have no hesitation in using it. > > I work with ERP, and throughout the time, the later, lock resources and > release them soon, the better, for the performance of the system as a whole. > > Even if it doesn't make much difference locally, using this process, > throughout the system, efficiency is noticeable. > > Apparently, it is more code, but it is less resources used and for less > time. > > And (2), if it is a case, frequently, no table would be blocked in this > function. > > Nobody here is going to question the concept that it's better to use > resources for less time rather than more, but the wisdom of sticking > to well-established coding patterns instead of inventing altogether > new ones is also well-understood. There are often good reasons why the > code is written in the way that it is, and it's important to > understand those before proposing to change things. > I see, the famous "cliché". regards, Ranier Vilela > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: Unify drop-by-OID functions
Em ter., 5 de mai. de 2020 às 13:06, Robert Haas
escreveu:
> On Fri, May 1, 2020 at 5:32 PM Ranier Vilela wrote:
> > I can suggest improvements?
> >
> > 1. In case Object is cached, delay open_table until the last moment, for
> the row to be blocked as little as possible and close the table as quickly
> as possible.
> > 2. In case Object is cached and the tuple is invalid, do not open table.
> > 3. Otherwise, is it possible to call systable_endscan, after table_close?
> >
> > I think that lock resources, for as little time as possible, it is an
> advantage..
>
> Only if it's correct, which (3) definitely wouldn't be, and I'm
> doubtful about (1) as well.
>
Ok, so the question. If (3) is not safe, obvious we shouldn't use, and must
call table_close, after systable_endscan.
Now (1) and (2), I would have no hesitation in using it.
I work with ERP, and throughout the time, the later, lock resources and
release them soon, the better, for the performance of the system as a whole.
Even if it doesn't make much difference locally, using this process,
throughout the system, efficiency is noticeable.
Apparently, it is more code, but it is less resources used and for less
time.
And (2), if it is a case, frequently, no table would be blocked in this
function.
Simple examples.
Exemple 1:
FILE * f;
f = fopen("data.txt", "r");
if (f != NULL) {
char buf[512];
size_t result;
result = fread(&buf, sizeof(char), 512, f);
fclose(f); // we no longer need the resource, release.
if (result != 0) {
process(buf);
printf("buf=%s\n", buf);
}
}
Exemple 2:
FILE * f;
f = fopen("data.txt", "r");
if (f != NULL) {
char buf[512];
size_t result;
result = fread(&buf, sizeof(char), 512, f);
if (result != 0) {
process(buf);
printf("buf=%s\n", buf);
}
fclose(f); // resource blocked until the end.
}
regards,
Ranier Vilela
Re: [REPORT] Static analys warnings
Fix possible overflow when converting, possible negative number to uint16. postingoff can be -1,when converts to uint16, overflow can raise. Otherwise, truncation can be occurs, losing precision, from int (31 bits) to uint16 (15 bits) There is a little confusion in the parameters of some functions in this file, postigoff is declared as int, other declared as uint16. src/backend/access/nbtree/nbtinsert.c static void _bt_insertonpg(Relation rel, BTScanInsert itup_key, Buffer buf, Buffer cbuf, BTStack stack, IndexTuple itup, Size itemsz, OffsetNumber newitemoff, int postingoff, // INT bool split_only_page); static Buffer _bt_split(Relation rel, BTScanInsert itup_key, Buffer buf, Buffer cbuf, OffsetNumber newitemoff, Size newitemsz, IndexTuple newitem, IndexTuple orignewitem, IndexTuple nposting, uint16 postingoff); // UINT16 regards, Ranier Vilela fix_possible_overflow_postingoff.patch Description: Binary data
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em seg., 4 de mai. de 2020 às 02:58, Michael Paquier escreveu: > On Sun, May 03, 2020 at 04:23:24PM -0300, Ranier Vilela wrote: > > I don't know if it applies to the same case, but from the moment I > > installed python on the development machine, the Postgres build stopped > > working correctly. > > Although perl, flex and bison are available in the path, the build does > not > > generate files that depend on flex and bison. > > Are you following the instructions of the documentation? Here is a > link to them: > https://www.postgresql.org/docs/devel/install-windows-full.html My environment was ok and worked 100%, compiling with msvc 2019 (64 bits). > > > My guess is that you would be just missing a PATH configuration or > such because python enforced a new setting? > Perl and flex and bison, are in the path, no doubt. > > > Warning from build.pl > > Use of uninitialized value $ARGV[0] in uc at build.pl line 44. > > Use of uninitialized value $ARGV[0] in uc at build.pl line 48. > > Hmm. We have buildfarm machines using the MSVC scripts and Python, > see for example woodloose. And note that @ARGV would be normally > defined, so your warning looks fishy to me. > I'll redo from the beginning. 1. Make empty directory postgres 2. Clone postgres 3. Call msvc 2019 (64 bits) env batch 4. Setup path to perl, bison and flex set path=%path%;c:\perl;\bin;c:\bin 5. C:\dll>perl -V Summary of my perl5 (revision 5 version 30 subversion 1) configuration: Platform: osname=MSWin32 osvers=10.0.18363.476 archname=MSWin32-x64-multi-thread uname='Win32 strawberry-perl 5.30.1.1 #1 Fri Nov 22 02:24:29 2019 x64' 6. C:\dll>bison -V bison (GNU Bison) 2.7 Written by Robert Corbett and Richard Stallman. 7. C:\dll>flex -V flex 2.6.4 8. cd\dll\postgres\src\tools\msvc 9. build results: ... PrepareForBuild: Creating directory ".\Release\postgres\". Creating directory ".\Release\postgres\postgres.tlog\". InitializeBuildStatus: Creating ".\Release\postgres\postgres.tlog\unsuccessfulbuild" because "AlwaysCreate" was specified. CustomBuild: Running bison on src/backend/bootstrap/bootparse.y 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running flex on src/backend/bootstrap/bootscanner.l 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running bison on src/backend/parser/gram.y 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running flex on src/backend/parser/scan.l 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running bison on src/backend/replication/repl_gram.y 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running flex on src/backend/replication/repl_scanner.l Running bison on src/backend/replication/syncrep_gram.y 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running flex on src/backend/replication/syncrep_scanner.l 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running bison on src/backend/utils/adt/jsonpath_gram.y Running flex on src/backend/utils/adt/jsonpath_scan.l 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. Running flex on src/backend/utils/misc/guc-file.l 'perl' nao é reconhecido como um comando interno ou externo, um programa operável ou um arquivo em lotes. C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(231,5): error MSB6006: "cmd.exe" exited with code 9009. [C:\dll\postgres\postgres.vcxproj] Done Building Project "C:\dll\postgres\postgres.vcxproj" (default targets) -- FAILED. Done Building Project "C:\dll\postgres\pgsql.sln" (default targets) -- FAILED. ... Build FAILED. "C:\dll\postgres\pgsql.sln" (default target) (1) -> "C:\dll\postgres\cyrillic_and_mic.vcxproj" (default target) (2) -> "C:\dll\postgres\postgres.vcxproj" (default target) (3) -> (CustomBuild target) -> C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(231,5): error MSB6006: "cmd.exe" exited
[REPORT] Static analys warnings
1. Warning: the right operand to | always evaluates to 0 src/include/storage/bufpage.h #define PAI_OVERWRITE (1 << 0) #define PAI_IS_HEAP (1 << 1) #define PageAddItem(page, item, size, offsetNumber, overwrite, is_heap) \ PageAddItemExtended(page, item, size, offsetNumber, \ ((overwrite) ? PAI_OVERWRITE : 0) | \ ((is_heap) ? PAI_IS_HEAP : 0)) Typo | should be ||: ((overwrite) ? PAI_OVERWRITE : 0) || \ ((is_heap) ? PAI_IS_HEAP : 0)) regards, Ranier Vilela
Re: Postgres Windows build system doesn't work with python installed in Program Files
Em qui., 30 de abr. de 2020 às 09:06, Victor Wagner
escreveu:
>
> Collegues,
>
> Accidently I've come over minor bug in the Mkvcbuild.pm.
> It happens, that it doesn't tolerate spaces in the $config->{python}
> path, because it want to call python in order to find out version,
> prefix and so on, and doesn't properly quote command.
>
> Fix is very simple, see attach.
>
> Patch is made against REL_12_STABLE, but probably applicable to other
> versions as well.
>
I don't know if it applies to the same case, but from the moment I
installed python on the development machine, the Postgres build stopped
working correctly.
Although perl, flex and bison are available in the path, the build does not
generate files that depend on flex and bison.
Running bison on src/backend/bootstrap/bootparse.y
'perl' nao é reconhecido como um comando interno
ou externo, um programa operável ou um arquivo em lotes.
Running flex on src/backend/bootstrap/bootscanner.l
'perl' nao é reconhecido como um comando interno
ou externo, um programa operável ou um arquivo em lotes.
Running bison on src/backend/parser/gram.y
'perl' nao é reconhecido como um comando interno
ou externo, um programa operável ou um arquivo em lotes.
etc
Warning from build.pl
Use of uninitialized value $ARGV[0] in uc at build.pl line 44.
Use of uninitialized value $ARGV[0] in uc at build.pl line 48.
regards,
Ranier Vilela
Re: Unify drop-by-OID functions
Em sáb., 2 de mai. de 2020 às 05:01, Peter Eisentraut < peter.eisentr...@2ndquadrant.com> escreveu: > On 2020-05-01 23:31, Ranier Vilela wrote: > > I can suggest improvements? > > > > 1. In case Object is cached, delay open_table until the last moment, for > > the row to be blocked as little as possible and close the table as > > quickly as possible. > > 2. In case Object is cached and the tuple is invalid, do not open table. > > 3. Otherwise, is it possible to call systable_endscan, after table_close? > > What do you mean by "object is cached"? > Well, that's what I deduced from the cacheId variable name. regards, Ranier Vilela
Re: Unify drop-by-OID functions
Em sex., 1 de mai. de 2020 às 11:39, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> escreveu:
> [proposal for PG 14]
>
> There are a number of Remove${Something}ById() functions that are
> essentially identical in structure and only different in which catalog
> they are working on. This patch refactors this to be one generic
> function. The information about which oid column, index, etc. to use
> was already available in ObjectProperty for most catalogs, in a few
> cases it was easily added.
>
> Conceivably, this could be taken further by categorizing more special
> cases as ObjectProperty fields or something like that, but this seemed
> like a good balance.
>
Very good.
I can suggest improvements?
1. In case Object is cached, delay open_table until the last moment, for
the row to be blocked as little as possible and close the table as quickly
as possible.
2. In case Object is cached and the tuple is invalid, do not open table.
3. Otherwise, is it possible to call systable_endscan, after table_close?
I think that lock resources, for as little time as possible, it is an
advantage..
+static void
+DropGenericById(const ObjectAddress *object)
+{
+ int cacheId;
+ Relation rel;
+ HeapTuple tup;
+
+ cacheId = get_object_catcache_oid(object->classId);
+
+ /*
+ * Use the system cache for the oid column, if one exists.
+ */
+ if (cacheId >= 0)
+ {
+ tup = SearchSysCache1(cacheId, ObjectIdGetDatum(object->objectId));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for %s entry %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+rel = table_open(object->classId, RowExclusiveLock);
+ CatalogTupleDelete(rel, &tup->t_self);
+table_close(rel, RowExclusiveLock);
+
+ ReleaseSysCache(tup);
+ }
+ else
+ {
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+
+ ScanKeyInit(&skey[0],
+ get_object_attnum_oid(object->classId),
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(object->objectId));
+
+rel = table_open(object->classId, RowExclusiveLock);
+ scan = systable_beginscan(rel, get_object_oid_index(object->classId),
true,
+ NULL, 1, skey);
+
+ /* we expect exactly one match */
+ tup = systable_getnext(scan);
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "could not find tuple for class %u entry %u",
+ object->classId, object->objectId);
+
+ CatalogTupleDelete(rel, &tup->t_self);
+ systable_endscan(scan);
+table_close(rel, RowExclusiveLock);
+ }
+}
+
regards,
Ranier Vilela
Re: [PATCH] Fix possible overflow on tuplesort.c
Em qui., 23 de abr. de 2020 às 16:43, Alvaro Herrera < alvhe...@2ndquadrant.com> escreveu: > On 2020-Apr-16, Ranier Vilela wrote: > > > When multiplying variables, the overflow will take place anyway, and only > > then will the meaningless product be explicitly promoted to type int64. > > It is one of the operands that should have been cast instead to avoid the > > overflow. > > > > - if (state->availMem < (int64) ((newmemtupsize - memtupsize) * > sizeof(SortTuple))) > > + if (state->availMem < ((int64) (newmemtupsize - memtupsize) * > sizeof(SortTuple))) > > Doesn't sizeof() return a 64-bit wide value already? > Sizeof return size_t. Both versions are constant expressions of type std::size_t <https://en.cppreference.com/w/cpp/types/size_t>. regards, Ranier Vilela
Re: [PATCH] FIx resource leaks (pg_resetwal.c)
Em qui., 23 de abr. de 2020 às 16:43, Robert Haas escreveu: > On Thu, Apr 23, 2020 at 2:41 PM Ranier Vilela wrote: > > I do not agree in any way. At the very least what I am reporting is > suspect. And if I already propose a solution even if it is not the best, it > is much better than being silent and missing the opportunity to fix a bug. > > Ridiculous is your lack of education. > > That's rather rude. I doubt that you know anything about how much > education Andres does nor does not have. The fact that he doesn't > agree with you does not mean that he is poorly educated. > Sorry Robert. > > On the substance of the issue, I see from the commit log that you've > gotten a few real issues fixed -- but I also agree with Andres that > you've reported a lot of things that are not real issues, and that > takes up other people's time looking at things that really don't > matter. Please make an effort not to report things that don't actually > need to be fixed. All my patches don't just leave my head. It comes from reports of analysis tools, by themselves, they are already suspect. I confess that FATAL error log, confused me a lot and since then, I have tried my best not to make the same mistakes. > > > pg_resetwal exits very quickly, generally in a small fraction of a > second. The allocation you're at pains to free only happens once per > execution and allocates only 8kB. Trying to free allocations that are > tiny and short-lived has no benefit. It's better to let the program > exit that much quicker, at which point all the memory is freed anyway. > Read_controlfile is a function, as it stands, it is useless to be reused. best regards, Ranier Vilela
Re: [PATCH] FIx resource leaks (pg_resetwal.c)
Em qui., 23 de abr. de 2020 às 16:27, Peter Geoghegan escreveu: > On Thu, Apr 23, 2020 at 11:41 AM Ranier Vilela > wrote: > > And if I already propose a solution even if it is not the best, it is > much better than being silent and missing the opportunity to fix a bug. > > The problem with that theory is that you're not creating any value > over simply running Coverity directly. Your patches don't seem to be > based on any real analysis beyond what makes Coverity stop > complaining, which is not helpful. > In some cases, this may be true. But in fact I already fixed some bugs with this technique. Even you already used a patch of mine to provide a fix. Wasn't that helpful? 1. https://www.postgresql.org/message-id/CAEudQAqJ%3DMVqJd4MHi%3DiMLismngE4GJqdiEZa1isxF3Pem-udg%40mail.gmail.com > For example, the nbtree.c/btvacuumpage() issue you reported yesterday > involved a NULL pointer dereference, but if the code path in question > ever dereferenced the NULL pointer then it would be fundamentally > wrong in many other ways, probably leading to data corruption. The fix > that you posted obviously completely missed the point. Even when > Coverity identifies a serious issue, it usually needs to be carefully > interpreted. > I disagree. In case of nbtree.c/btvacuumpag(). If you are validating "opaque" pointer, in three different ways to proceed with cleaning, nothing more correct than validating the most important first, if the pointer is really valid. And that is what the patch does. > > Anybody can run Coverity. Many of us do. Maybe the approach you've > taken would have had a noticeable benefit if you were not dealing with > a codebase that has already been subject to lots of triage of Coverity > issues. > Sorry, but the plsql-bugs list, has many reports of segmentation faults, that shouldn't exist, if everyone uses Coverity or other tools, after writing the code. > > Ridiculous is your lack of education. > > This isn't helping you at all. > Consideration and respect first. regards, Ranier Vilela
[PATCH] Fix possible Uninitialized variables (parse_manifest.c)
Hi, Per Coverity. verify_manifest_checksum, declare and can utilize array of uint8, without initializing it. While here, I applied the quick exit technique, to avoid unnecessary computations, if it is possible to avoid them. regards, Ranier Vilela fix_uninitialized_array_parse_manifest.patch Description: Binary data
Re: [PATCH] FIx resource leaks (pg_resetwal.c)
Em qui., 23 de abr. de 2020 às 15:27, Andres Freund
escreveu:
> Hi,
>
> On 2020-04-23 15:20:59 -0300, Ranier Vilela wrote:
> > Per Coverity.
> >
> > read_controlfile alloc memory with pg_malloc and fail in releasing the
> > memory.
>
> Seriously, this is getting really ridiculous. You're posting badly
> vetted, often nearly verbatim, coverity reports. Many of them are
> obvious false positives. This is just producing noise.
>
I do not agree in any way. At the very least what I am reporting is
suspect. And if I already propose a solution even if it is not the best, it
is much better than being silent and missing the opportunity to fix a bug.
Ridiculous is your lack of education.
>
> Please stop.
>
I will ignore.
> > diff --git a/src/bin/pg_resetwal/pg_resetwal.c
> b/src/bin/pg_resetwal/pg_resetwal.c
> > index 233441837f..673ab0204c 100644
> > --- a/src/bin/pg_resetwal/pg_resetwal.c
> > +++ b/src/bin/pg_resetwal/pg_resetwal.c
> > @@ -608,6 +608,7 @@ read_controlfile(void)
> > len = read(fd, buffer, PG_CONTROL_FILE_SIZE);
> > if (len < 0)
> > {
> > + pg_free(buffer);
> > pg_log_error("could not read file \"%s\": %m",
> XLOG_CONTROL_FILE);
> > exit(1);
> > }
>
> There's an exit() two lines later, this is obviously not necessary.
>
Excess.
Did you read patch all over?
memcpy(&ControlFile, buffer, sizeof(ControlFile));
+ pg_free(buffer);
/* return false if WAL segment size is not valid */
if (!IsValidWalSegSize(ControlFile.xlog_seg_size))
@@ -644,6 +646,7 @@ read_controlfile(void)
return true;
}
+pg_free(buffer);
/* Looks like it's a mess. */
pg_log_warning("pg_control exists but is broken or wrong version;
ignoring it");
Report for Coverity:
*** CID 1425435: Resource leaks (RESOURCE_LEAK)
/dll/postgres/src/bin/pg_resetwal/pg_resetwal.c: 650 in read_controlfile()
644
645 return true;
646 }
647
648 /* Looks like it's a mess. */
649 pg_log_warning("pg_control exists but is broken or wrong version;
ignoring it");
>>> CID 1425435: Resource leaks (RESOURCE_LEAK)
>>> Variable "buffer" going out of scope leaks the storage it points to.
650 return false;
651 }
652
653
654 /*
655 * Guess at pg_control values when we can't read
regards,
Ranier Vilela
fix_resource_leaks_pg_resetwal_v2.patch
Description: Binary data
[PATCH] FIx resource leaks (pg_resetwal.c)
Hi, Per Coverity. read_controlfile alloc memory with pg_malloc and fail in releasing the memory. regards, Ranier Vilela fix_resource_leaks_pg_resetwal.patch Description: Binary data
[PATCH] Fix Null pointer dereferences (pgoutput.c)
Hi, Per Coverity. If test oldtuple can be NULL, I mean it can really be NULL. On DELETE process, if oldtuple is NULL, log error and continue. So UPDATE must have the same treatment. regards, Ranier Vilela fix_null_pointer_dereference_pgoutput.patch Description: Binary data
[PATCH] Fix division by zero (explain.c)
Hi, Per Coverity. If has 0 full groups, "we don't need to do anything" and need goes to next. Otherwise a integer division by zero, can raise. comments extracted trom explain.c: /* * Since we never have any prefix groups unless we've first sorted * a full groups and transitioned modes (copying the tuples into a * prefix group), we don't need to do anything if there were 0 full * groups. */ regards, Ranier Vilela fix_division_by_zero_explain.patch Description: Binary data
Re: [PATCH] Fix buffer not null terminated on (ecpg lib)
Em qua., 22 de abr. de 2020 às 23:27, Kyotaro Horiguchi < horikyota@gmail.com> escreveu: > Hello. > > At Wed, 22 Apr 2020 19:48:07 -0300, Ranier Vilela > wrote in > > Hi, > > strncpy, it is not a safe function and has the risk of corrupting memory. > > On ecpg lib, two sources, make use of strncpy risk, this patch tries to > fix. > > > > 1. Make room for the last null-characte; > > 2. Copies Maximum number of characters - 1. > > > > per Coverity. > > - strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate)); > + sqlca->sqlstate[sizeof(sqlca->sqlstate) - 1] = '\0'; > + strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate) - 1); > > Did you look at the definition and usages of the struct member? > sqlstate is a char[5], which is to be filled with 5-letter SQLSTATE > code not terminated by NUL, which can be shorter if NUL is found > anywhere (I'm not sure there's actually a case of a shorter state > code). If you put NUL to the 5th element of the array, you break the > content. The existing code looks perfect to me. > Sorry, you are right. > > - strncpy(sqlca->sqlerrm.sqlerrmc, message, > sizeof(sqlca->sqlerrm.sqlerrmc)); > - sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] = 0; > + sqlca->sqlerrm.sqlerrmc[sizeof(sqlca->sqlerrm.sqlerrmc) - 1] = > '\0'; > + strncpy(sqlca->sqlerrm.sqlerrmc, message, > sizeof(sqlca->sqlerrm.sqlerrmc) - 1); > > The existing strncpy then terminating by NUL works fine. I don't think > there's any point in doing the reverse way. Actually > sizeof(sqlca->sqlerrm.sqlerrmc) - 1 is enough for the length but the > existing code is not necessarily a bug. > Without understanding then, why Coveriy claims bug here. regards, Ranier Vilela
Re: [PATCH] FIx explicit null dereference pointer (nbtree.c)
Em qua., 22 de abr. de 2020 às 21:24, Peter Geoghegan escreveu: > On Wed, Apr 22, 2020 at 3:55 PM Ranier Vilela wrote: > > per Coverity. > > Some Postgres hackers have access to a dedicated coverity > installation, and this issue has probably already been dismissed. > I will take a note. > > > 1. assign_zero: Assigning: opaque = NULL. > > 2. Condition buf < 0, taking true branch. > > 3. Condition !(((PageHeader)page)->pd_upper == 0), taking false branch. > > 4. Condition blkno != orig_blkno, taking true branch. > > 5. Condition _bt_page_recyclable(page), taking false branch. > > CID 1314742 (#2 of 2): Explicit null dereferenced (FORWARD_NULL) > > 6. var_deref_op: Dereferencing null pointer opaque. > > This is a false positive. btvacuumpage() is supposed to be a recursive > function, but in practice the only caller always uses the same block > number for both blkno and orig_blkno -- the tail recursion is actually > implemented using goto/a loop. > This means that it is impossible for these conditions described by Coverity to happen on the first call, when the var opaque is NULL. regards, Ranier Vilela
[PATCH] FIx explicit null dereference pointer (nbtree.c)
Hi, per Coverity. 1. assign_zero: Assigning: opaque = NULL. 2. Condition buf < 0, taking true branch. 3. Condition !(((PageHeader)page)->pd_upper == 0), taking false branch. 4. Condition blkno != orig_blkno, taking true branch. 5. Condition _bt_page_recyclable(page), taking false branch. CID 1314742 (#2 of 2): Explicit null dereferenced (FORWARD_NULL) 6. var_deref_op: Dereferencing null pointer opaque. regards, Ranier Vilela fix_explicit_null_dereference_nbtree.patch Description: Binary data
[PATCH] Fix buffer not null terminated on (ecpg lib)
Hi, strncpy, it is not a safe function and has the risk of corrupting memory. On ecpg lib, two sources, make use of strncpy risk, this patch tries to fix. 1. Make room for the last null-characte; 2. Copies Maximum number of characters - 1. per Coverity. regards, Ranier Vilela fix_buffer_not_null_terminated.patch Description: Binary data
Re: PG compilation error with Visual Studio 2015/2017/2019
Em qua., 22 de abr. de 2020 às 08:43, Amit Kapila escreveu: > On Tue, Apr 21, 2020 at 5:32 PM Juan José Santamaría Flecha > wrote: > > > > On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila > wrote: > >> > 6. I have additionally done some cosmetic changes in the attached patch. > I made some style changes too. 1. Change: strcpy(iso_lc_messages, "C"); to iso_lc_messages[0] = 'C'; iso_lc_messages[1] = '\0'; 2. Remove vars hyphen and underscore; 3. Avoid call second wcsrchr, if hyphen is not found. If it's not too much perfectionism. regards, Ranier Vilela 0001-PG-compilation-error-with-VS-2015-2017-2019_v12.patch Description: Binary data
Re: PG compilation error with Visual Studio 2015/2017/2019
Em ter., 21 de abr. de 2020 às 09:02, Juan José Santamaría Flecha < juanjo.santama...@gmail.com> escreveu: > > On Tue, Apr 21, 2020 at 12:41 PM Amit Kapila > wrote: > >> On Tue, Apr 21, 2020 at 12:51 PM Amit Kapila >> wrote: >> > >> > I have tried a simple test with the latest patch and it failed for me. >> > >> > Set LC_MESSAGES="English_United Kingdom"; >> > -- returns en-GB, then code changes it to en_NZ when _create_locale() >> > is used whereas with the patch it returns "" (empty string). >> > >> > There seem to be two problems here (a) The input to enum_locales_fn >> > doesn't seem to get the input name as "English_United Kingdom" due to >> > which it can't find a match even if the same exists. (b) After >> > executing EnumSystemLocalesEx, there is no way the patch can detect if >> > it is successful in finding the passed name due to which it appends >> > empty string in such cases. >> >> Few more comments: >> 1. I have tried the first one in the list provided by you and that >> also didn't work. Basically, I got empty string when I tried Set >> LC_MESSAGES='Afar'; >> > > I cannot reproduce any of these errors on my end. When using > _create_locale(), returning "en_NZ" is also a wrong result. > > >> 2. Getting below warning >> pg_locale.c(1072): warning C4133: 'function': incompatible types - >> from 'const char *' to 'const wchar_t *' >> > > Yes, that is a regression. > > >> 3. >> + if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHCOUNTRYNAME, >> + test_locale + len, LOCALE_NAME_MAX_LENGTH - len) > 0) >> >> All > or <= 0 checks should be changed to "!" types which mean to >> check whether the call toGetLocaleInfoEx is success or not. >> > > MSVC does not recommend "!" in all cases, but GetLocaleInfoEx() looks > fine, so agreed. > > 4. In the patch, first, we try to get with LCType as LOCALE_SNAME and >> then with LOCALE_SENGLISHLANGUAGENAME and LOCALE_SENGLISHCOUNTRYNAME. >> I think we should add comments indicating why we try to get the locale >> information with three LCTypes and why the specific order of trying >> those types is required. >> > > Agreed. > > >> 5. In one of the previous emails, you asked whether we have a list of >> supported locales. I don't find any such list. I think it depends on >> Windows locales for which you can get the information from >> >> https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-lcid/a9eac961-e77d-41a6-90a5-ce1a8b0cdb9c > > > Yes, that is the information we get from EnumSystemLocalesEx(), without > the additional entries _create_locale() has. > > Please find attached a new version addressing the above mentioned, and so > adding a debug message for trying to get more information on the failed > cases. > More few comments. 1. Comments about order: /* * Callback function for EnumSystemLocalesEx. * Stop enumerating if a match is found for a locale with the format * _. * The order for search locale is essential: * Find LCType first as LOCALE_SNAME, if not found try LOCALE_SENGLISHLANGUAGENAME and * finally LOCALE_SENGLISHCOUNTRYNAME, before return. */ Typo "enumarating". 2. Maybe the fail has here: if (hyphen == NULL || underscore == NULL) Change || to &&, the logical is wrong? 3. Why iso_lc_messages[0] = '\0'? If we go call strchr, soon after, it's a waste. regards, Ranier Vilela
Re: [PATCH] Small optimization across postgres (remove strlen duplicate usage)
Em dom., 19 de abr. de 2020 às 22:00, David Rowley
escreveu:
> On Mon, 20 Apr 2020 at 11:24, Ranier Vilela wrote:
> > I tried: https://godbolt.org with:
> >
> > -O2:
> >
> > f1:
> > int main (int argv, char **argc)
> > {
> > return strlen(argc[0]) == 0;
> > }
> >
> > f1: Assembly
> > main: # @main
> > mov rcx, qword ptr [rsi]
> > xor eax, eax
> > cmp byte ptr [rcx], 0
> > seteal
> > ret
> >
> > f2:
> > int main (int argv, char **argc)
> > {
> > return argc[0] == '\0';
> > }
> >
> > f2: Assembly
> >
> > main: # @main
> > xor eax, eax
> > cmp qword ptr [rsi], 0
> > seteal
> > ret
> >
> > For me clearly str [0] == '\ 0', wins.
>
> I think you'd want to use argc[0][0] == '\0' or *argc[0] == '\0'.
> Otherwise you appear just to be checking if the first element in the
> argc pointer array is set to NULL, which is certainly not the same as
> an empty string.
>
I guess you're right.
x86-64 clang (trunk) -O2
f1:
int cmp(const char * name)
{
return strlen(name) == 0;
}
cmp:# @cmp
xor eax, eax
cmp byte ptr [rdi], 0
seteal
ret
f2:
int cmp(const char * name)
{
return name[0] == '\0';
}
cmp: # @cmp
xor eax, eax
cmp byte ptr [rdi], 0
seteal
ret
Is the same result in assembly.
Well, it doesn't matter to me, I will continue to use str[0] == '\0'.
Thanks for take part.
regards,
Ranier VIlela
Re: [PATCH] Small optimization across postgres (remove strlen duplicate usage)
Em dom., 19 de abr. de 2020 às 18:38, Tom Lane escreveu:
> Tomas Vondra writes:
> > On Sun, Apr 19, 2020 at 11:24:38AM -0300, Ranier Vilela wrote:
> >> strlen it is one of the low fruits that can be harvested.
>
> > Maybe there are places where this would help, but I don't see a reason
> > to just throw away all strlen calls and replace them with something
> > clearly less convenient and possibly more error-prone (I'd expect quite
> > a few off-by-one mistakes with this).
>
> I've heard it claimed that modern compilers will optimize
> strlen('literal') to a constant at compile time. I'm not sure how
> much I believe that, but to the extent that it's true, replacing such
> calls would provide exactly no performance benefit.
>
Tom, I wouldn't believe it very much, they still have a lot of stupid
compilers out there (msvc for example).
Furthermore, optimizations are a complicated business, often the adjacent
code does not allow for such optimizations.
When a programmer does, there is no doubt.
>
> I'm quite -1 on changing these to sizeof(), in any case, because
> (a) that opens up room for confusion about whether the trailing nul is
> included, and (b) it makes it very easy, when changing or copy/pasting
> code, to apply sizeof to something that's not a string literal, with
> disastrous results.
>
It may be true, but I have seen a lot of Postgres code, where sizeof is
used extensively, even with real chances of what you said happened. So that
risk already exists.
>
> The cases where Ranier proposes to replace strlen(foo) == 0
> with a test on foo[0] do seem like wins, though. Asking for
> the full string length to be computed is more computation than
> necessary, and it's less clear that the compiler could be
> expected to save you from that. Anyway there's a coding style
> proposition that we should be doing this consistently, and
> certainly lots of places do do this without using strlen().
>
Yes, this is the idea.
>
> I can't get excited about the proposed changes to optimize away
> multiple calls of strlen() either, unless there's performance
> measurements to support them individually. This again seems like
> something a compiler might do for you.
>
Again, the compiler will not always save us.
I have seen many fanstatic solutions in Postgres, but I have also seen a
lot of written code, forgive me, without caprice, without much care.
The idea is, little by little, to prevent carefree code, either written or
left in Postgres.
You can see an example in that patch.
After a few calls the programmer validates the entry and leaves if it is
bad. When it should be done before anything.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5bdc02fce2..a00cca2605 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10699,10 +10699,15 @@ GUCArrayDelete(ArrayType *array, const char *name)
struct config_generic *record;
ArrayType *newarray;
int i;
+ int len;
int index;
Assert(name);
+ /* if array is currently null, then surely nothing to delete */
+ if (!array)
+ return NULL;
+
/* test if the option is valid and we're allowed to set it */
(void) validate_option_array_item(name, NULL, false);
@@ -10711,12 +10716,9 @@ GUCArrayDelete(ArrayType *array, const char *name)
if (record)
name = record->name;
- /* if array is currently null, then surely nothing to delete */
- if (!array)
- return NULL;
-
newarray = NULL;
index = 1;
+ len = strlen(name);
for (i = 1; i <= ARR_DIMS(array)[0]; i++)
{
@@ -10735,8 +10737,8 @@ GUCArrayDelete(ArrayType *array, const char *name)
val = TextDatumGetCString(d);
/* ignore entry if it's what we want to delete */
- if (strncmp(val, name, strlen(name)) == 0
- && val[strlen(name)] == '=')
+ if (strncmp(val, name, len) == 0
+ && val[len] == '=')
continue;
/* else add it to the output array */
regards,
Ranier Vilela
Re: [PATCH] Small optimization across postgres (remove strlen duplicate usage)
Em dom., 19 de abr. de 2020 às 19:00, David Rowley
escreveu:
> On Mon, 20 Apr 2020 at 09:38, Tom Lane wrote:
> > The cases where Ranier proposes to replace strlen(foo) == 0
> > with a test on foo[0] do seem like wins, though. Asking for
> > the full string length to be computed is more computation than
> > necessary, and it's less clear that the compiler could be
> > expected to save you from that. Anyway there's a coding style
> > proposition that we should be doing this consistently, and
> > certainly lots of places do do this without using strlen().
>
> Looking at https://godbolt.org/z/6XsjbA it seems like GCC is pretty
> good at getting rid of the strlen call even at -O0. It takes -O1 for
> clang to use it and -O2 for icc.
>
I tried: https://godbolt.org with:
-O2:
f1:
int main (int argv, char **argc)
{
return strlen(argc[0]) == 0;
}
f1: Assembly
main: # @main
mov rcx, qword ptr [rsi]
xor eax, eax
cmp byte ptr [rcx], 0
seteal
ret
f2:
int main (int argv, char **argc)
{
return argc[0] == '\0';
}
f2: Assembly
main: # @main
xor eax, eax
cmp qword ptr [rsi], 0
sete al
ret
For me clearly str [0] == '\ 0', wins.
regards,
Ranier Vilela
Re: [PATCH] Small optimization across postgres (remove strlen duplicate usage)
Em dom., 19 de abr. de 2020 às 16:33, Tomas Vondra < tomas.von...@2ndquadrant.com> escreveu: > On Sun, Apr 19, 2020 at 11:24:38AM -0300, Ranier Vilela wrote: > >Hi, > >strlen it is one of the low fruits that can be harvested. > >What is your opinion? > > > > That assumes this actually affects/improves performance, without any > measurements proving that. Considering large number of the places you > modified are related to DDL (CreateComment, ChooseIndexColumnNames, ...) > or stuff that runs only once or infrequently (like the changes in > PostmasterMain or libpqrcv_get_senderinfo). Likewise, it seems entirely > pointless to worry about strlen() overhead e.g. in fsync_parent_path > which is probably dominated by I/O. > With code as interconnected as postgres, it is difficult to say that a function, which calls strlen, repeatedly, would not have any gain. Regarding the functions, I was just being consistent, trying to remove all occurrences, even where, there is very little gain. > > Maybe there are places where this would help, but I don't see a reason > to just throw away all strlen calls and replace them with something > clearly less convenient and possibly more error-prone (I'd expect quite > a few off-by-one mistakes with this). > Yes, always, it is prone to errors, but for the most part, they are safe changes. It passes all 199 tests, of course it has not been tested in a real production environment. Perhaps the time is not the best, the end of the cycle, but, once done, I believe it would be a good harvest. Thank you for comment. regards, Ranier Vilela
Re: PG compilation error with Visual Studio 2015/2017/2019
Em dom., 19 de abr. de 2020 às 12:34, Juan José Santamaría Flecha < juanjo.santama...@gmail.com> escreveu: > > On Sun, Apr 19, 2020 at 1:58 PM Ranier Vilela wrote: > >> Em dom., 19 de abr. de 2020 às 07:16, Juan José Santamaría Flecha < >> juanjo.santama...@gmail.com> escreveu: >> >>> On Sat, Apr 18, 2020 at 1:43 PM Ranier Vilela >>> wrote: >>> >>>> I have some observations about this patch, related to style, if you >>>> will allow me. >>>> >>> Please find attached a revised version. >>> >> Looks good to me, but, sorry, I think I missed a glitch in the previous >> review. >> If _create_locale fail, no need to call _free_locale(loct);. >> >> Another point is, what is the difference between pg_mbstrlen and wcslen? >> It would not be better to use only wcslen? >> > > pg_mbstrlen() is for multibyte strings and wcslen() is for wide-character > strings, the "pg" equivalent would be pg_wchar_strlen(). > > Attached have the patch with this comments. >> > > + } else > > This line needs a break, other than that LGTM. > Sure. Attached new patch with this revision. regards, Ranier Vilela 0001-PG-compilation-error-with-VS-2015-2017-2019_v09.patch Description: Binary data
[PATCH] Small optimization across postgres (remove strlen duplicate usage)
Hi, strlen it is one of the low fruits that can be harvested. What is your opinion? regards, Ranier Vilela remove_strlen.patch Description: Binary data
Re: PG compilation error with Visual Studio 2015/2017/2019
Em dom., 19 de abr. de 2020 às 07:16, Juan José Santamaría Flecha <
juanjo.santama...@gmail.com> escreveu:
>
> On Sat, Apr 18, 2020 at 6:07 AM Amit Kapila
> wrote:
>
>> On Sat, Apr 18, 2020 at 12:14 AM Juan José Santamaría Flecha
>> wrote:
>> >
>> > We can get a match for those locales in non-ISO format by enumerating
>> available locales with EnumSystemLocalesEx(), and trying to find a match.
>>
>> I have not reviewed or tested the new patch but one thing I would like
>> to see is the impact of setting LC_MESAGGES with different locale
>> information. Basically, the error messages after setting the locale
>> with _create_locale and with the new method being discussed. This
>> will help us in ensuring that we didn't break anything which was
>> working with prior versions of MSVC. Can you or someone try to test
>> and share the results of the same?
>>
>
> I cannot find a single place where all supported locales are listed, but I
> have created a small test program (WindowsNLSLocales.c) based on:
> [_] format locales [1], additional supported language
> strings [2], and additional supported country and region strings [3]. Based
> on the results from this test program, it is possible to to do a good job
> with the [_] types using the proposed logic, but the
> two later cases are Windows specific, and there is no way arround a
> lookup-table.
>
> The attached results (WindowsNLSLocales.ods) come from Windows 10 (1903)
> and Visual C++ build 1924, 64-bit.
>
> On Sat, Apr 18, 2020 at 1:43 PM Ranier Vilela wrote:
>
>> I have some observations about this patch, related to style, if you will
>> allow me.
>>
>
> Please find attached a revised version.
>
Looks good to me, but, sorry, I think I missed a glitch in the previous
review..
+#else /* _WIN32_WINNT < 0x0600 */
+ _locale_t loct;
+
+ loct = _create_locale(LC_CTYPE, winlocname);
+ if (loct != NULL)
+{
+ rc = wchar2char(iso_lc_messages, loct->locinfo->locale_name[LC_CTYPE],
+ sizeof(iso_lc_messages), NULL);
_free_locale(loct);
+}
+#endif /* _WIN32_WINNT >= 0x0600 */
If _create_locale fail, no need to call _free_locale(loct);.
Another point is, what is the difference between pg_mbstrlen and wcslen?
It would not be better to use only wcslen?
Attached have the patch with this comments.
regards,
Ranier Vilela
0001-PG-compilation-error-with-VS-2015-2017-2019_v08.patch
Description: Binary data
Re: PG compilation error with Visual Studio 2015/2017/2019
Em sex., 17 de abr. de 2020 às 15:44, Juan José Santamaría Flecha <
juanjo.santama...@gmail.com> escreveu:
>
> On Fri, Apr 17, 2020 at 10:33 AM Amit Kapila
> wrote:
>
>>
>> I see some differences in the output when _create_locale() is used vs.
>> when GetLocaleInfoEx() is used. Forex.
>>
>
> Thanks for the thorough review.
>
>
>> The function IsoLocaleName() header comment says "Convert a Windows
>> setlocale() argument to a Unix-style one", so I was expecting above
>> cases which gives valid values with _create_locale() should also work
>> with GetLocaleInfoEx(). If it is fine for GetLocaleInfoEx() to return
>> an error for the above cases, then we need an explanation of the same
>> and probably add a few comments as well. So, I am not sure if we can
>> conclude that GetLocaleInfoEx() is an alternative to _create_locale()
>> at this stage.
>>
>
> We can get a match for those locales in non-ISO format by enumerating
> available locales with EnumSystemLocalesEx(), and trying to find a match.
>
> Please find a new patch for so.
>
I have some observations about this patch, related to style, if you will
allow me.
1. argv variable on function enum_locales_fn can be reduced.
2. Var declaration len escapes the Postgres style.
3. Why call wcslen(test_locale), again, when var len have the size?
+static BOOL CALLBACK
+enum_locales_fn(LPWSTR pStr, DWORD dwFlags, LPARAM lparam)
+{
+ WCHAR test_locale[LOCALE_NAME_MAX_LENGTH];
+
+ memset(test_locale, 0, sizeof(test_locale));
+ if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHLANGUAGENAME,
+ test_locale, LOCALE_NAME_MAX_LENGTH) > 0)
+ {
+ size_t len;
+
+ wcscat(test_locale, L"_");
+ len = wcslen(test_locale);
+ if (GetLocaleInfoEx(pStr, LOCALE_SENGLISHCOUNTRYNAME,
+ test_locale + len, LOCALE_NAME_MAX_LENGTH - len) > 0)
+ {
+ WCHAR **argv = (WCHAR **) lparam;
+
+ if (wcsncmp(argv[0], test_locale, len) == 0)
+ {
+ wcscpy(argv[1], pStr);
+ return FALSE;
+ }
+ }
+ }
+ return TRUE;
+}
regards,
Ranier Vilela
[PATCH] Fix possible overflow on tuplesort.c
Hi, When multiplying variables, the overflow will take place anyway, and only then will the meaningless product be explicitly promoted to type int64. It is one of the operands that should have been cast instead to avoid the overflow. regards, Ranier Vilela tuplesort_avoid_possible_overflow.patch Description: Binary data
[PATCH] Tiny optimization on nbtinsert.c
Hi, Avoiding some calls and set vars, when it is not necessary. best regards, Ranier Vilela nbtinsert_tiny_optimization.patch Description: Binary data
Re: [PATCH'] Variables assigned with values that is never used.
Em sáb., 28 de mar. de 2020 às 10:33, Ranier Vilela
escreveu:
> Hi,
>
> Theses variables, are assigned with values that never is used and, can
> safely have their values removed.
>
1.
https://github.com/postgres/postgres/commit/f0ca378d4c139eda99ef14998115c1674dac3fc5
diff --git a/src/backend/access/nbtree/nbtsplitloc.c
b/src/backend/access/nbtree/nbtsplitloc.c
index 8ba055be9e..15ac106525 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -812,7 +812,6 @@ _bt_bestsplitloc(FindSplitData *state, int
perfectpenalty,
if (penalty <= perfectpenalty)
{
- bestpenalty = penalty;
lowsplit = i;
break;
}
Coincidence? I think not.
regards,
Ranier Vilela
Re: PG compilation error with Visual Studio 2015/2017/2019
Em qua., 15 de abr. de 2020 às 15:28, Juan José Santamaría Flecha < juanjo.santama...@gmail.com> escreveu: > > > On Wed, Apr 15, 2020 at 4:46 PM Ranier Vilela wrote: > >> Em qua., 15 de abr. de 2020 às 03:08, davinder singh < >> davindersingh2...@gmail.com> escreveu: >> >>> >>> 5. Why call _create_locale if _WIN32_WINNT >= 0x0600 is true and loct is >>>> not used? >>>> >>> _create_locale can take bigger input than GetLocaleInfoEx. But we are >>> interested in >>> *language[_country-region[.code-page]]*. We are using _create_locale to >>> validate >>> the given input. The reason is we can't verify the locale name if it is >>> appended with >>> code-page by using GetLocaleInfoEx. So before parsing, we verify if the >>> whole input >>> locale name is valid by using _create_locale. I hope that answers your >>> question. >>> >> Understood. In this case, _create_locale, is being used only to validate >> the input. >> Perhaps, in addition, you could create an additional function, which only >> validates winlocname, without having to create structures or use malloc, to >> be used when _WIN32_WINNT> = 0x0600 is true, but it is only a suggestion, >> if you think it is necessary. >> > > Looking at the comments for IsoLocaleName() I see: "MinGW headers declare > _create_locale(), but msvcrt.dll lacks that symbol". This is outdated > [1][2], and _create_locale() could be used from Windows 8, but I think we > should use GetLocaleInfoEx() as a complete alternative to _create_locale(). > Sounds good to me, the exception maybe log error in case fail? regards, Ranier Vilela
Re: PG compilation error with Visual Studio 2015/2017/2019
Em qua., 15 de abr. de 2020 às 03:08, davinder singh <
davindersingh2...@gmail.com> escreveu:
>
> Thanks for the review comments.
>
> On Tue, Apr 14, 2020 at 9:12 PM Ranier Vilela wrote:
>
>> >>I m still working on testing this patch. If anyone has Idea please
>> suggest.
>> I still see problems with this patch.
>>
>> 1. Variable loct have redundant initialization, it would be enough to
>> declare so: _locale_t loct;
>> 2. Style white space in variable rc declaration.
>> 3. Style variable cp_index can be reduced.
>> if (tmp != NULL) {
>>size_t cp_index;
>>
>> cp_index = (size_t)(tmp - winlocname);
>> strncpy(loc_name, winlocname, cp_index);
>> loc_name[cp_index] = '\0';
>> 4. Memory leak if _WIN32_WINNT >= 0x0600 is true, _free_locale(loct); is
>> not called.
>>
> I resolved the above comments.
>
Ok, thanks.
>
>> 5. Why call _create_locale if _WIN32_WINNT >= 0x0600 is true and loct is
>> not used?
>>
> _create_locale can take bigger input than GetLocaleInfoEx. But we are
> interested in
> *language[_country-region[.code-page]]*. We are using _create_locale to
> validate
> the given input. The reason is we can't verify the locale name if it is
> appended with
> code-page by using GetLocaleInfoEx. So before parsing, we verify if the
> whole input
> locale name is valid by using _create_locale. I hope that answers your
> question.
>
Understood. In this case, _create_locale, is being used only to validate
the input.
Perhaps, in addition, you could create an additional function, which only
validates winlocname, without having to create structures or use malloc, to
be used when _WIN32_WINNT> = 0x0600 is true, but it is only a suggestion,
if you think it is necessary.
But have a last problem, in case GetLocaleInfoEx fail, there is still one
memory leak,
before return NULL is needed call: _free_locale(loct);
Another suggestion, if GetLocaleInfoEx fail wouldn't it be good to log the
error and the error number?
regards,
Ranier Vilela
PG compilation error with Visual Studio 2015/2017/2019
>>I m still working on testing this patch. If anyone has Idea please
suggest.
I still see problems with this patch.
1. Variable loct have redundant initialization, it would be enough to
declare so: _locale_t loct;
2. Style white space in variable rc declaration.
3. Style variable cp_index can be reduced.
if (tmp != NULL) {
size_t cp_index;
cp_index = (size_t)(tmp - winlocname);
strncpy(loc_name, winlocname, cp_index);
loc_name[cp_index] = '\0';
4. Memory leak if _WIN32_WINNT >= 0x0600 is true, _free_locale(loct); is
not called.
5. Why call _create_locale if _WIN32_WINNT >= 0x0600 is true and loct is
not used?
regards,
Ranier Vilela
Re: PG compilation error with Visual Studio 2015/2017/2019
Em qui., 9 de abr. de 2020 às 09:14, Juan José Santamaría Flecha < juanjo.santama...@gmail.com> escreveu: > > On Thu, Apr 9, 2020 at 1:56 PM Ranier Vilela wrote: > >> Attached, your patch with those considerations. >> > I see no attachment. > Sorry, my mystake. regards, Ranier Vilela 0001-PG-compilation-error-with-VS-2015-2017-2019.patch Description: Binary data
Re: PG compilation error with Visual Studio 2015/2017/2019
>On Wed, Apr 8, 2020 at 7:39 PM Juan José Santamaría Flecha
>> Let me explain further, in pg_config_os.h you can check that the value of
>> _WIN32_WINNT is solely based on checking _MSC_VER. This patch should also
>> be meaningful for WIN32 builds using MinGW, or we might see this issue
>> reappear in those systems if update the MIN_WINNT value to more current
>> OS versions. So, I still think _WIN32_WINNT is a better option.
>>
>Thanks for explanation, I was not aware of that, you are right it make
>sense to use " _WIN32_WINNT", Now I am using this only.
>I still see the same last lines in both #ifdef blocks, and pgindent might
>> change a couple of lines to:
>> + MultiByteToWideChar(CP_ACP, 0, winlocname, -1, wc_locale_name,
>> + LOCALE_NAME_MAX_LENGTH);
>> +
>> + if ((GetLocaleInfoEx(wc_locale_name, LOCALE_SNAME,
>> + (LPWSTR)&buffer, LOCALE_NAME_MAX_LENGTH)) > 0)
>> + {
>>
>Now I have resolved these comments also, Please check updated version of
>the patch.
>> Please open an item in the commitfest for this patch.
>>
>I have created with same title.
Hi,
I have a few comments about the patch, if I may.
1. Variable rc runs the risk of being used uninitialized.
2. Variable loct has a redundant declaration ( = NULL).
3. Return "C", does not solve the first case?
Attached, your patch with those considerations.
regards,
Ranier VIlela
Re: [PATCH] Redudant initilization
Hi, New patch with yours suggestions. best regards, Ranier Vilela v2_redundant_initialization.patch Description: Binary data
[PATCH] Fix type var declaration (src/backend/access/brin/brin.c)
Hi, bringetbitmap function returns int64 value, but internally uses int. For prevent future bugs, fix to right type. best regards, Ranier Vilela fix_int64_brin.patch Description: Binary data
Re: [PATCH] remove condition always true (/src/backend/access/heap/vacuumlazy.c)
Hi, Thanks for the commit. Ranier Vilela
Re: [PATCH] remove condition always true (/src/backend/access/heap/vacuumlazy.c)
Em seg., 30 de mar. de 2020 às 18:14, Andres Freund escreveu: > Hi, > > On 2020-03-30 14:10:29 -0700, Andres Freund wrote: > > On 2020-03-30 17:08:01 -0300, Ranier Vilela wrote: > > > Em seg., 30 de mar. de 2020 às 16:05, Andres Freund < > and...@anarazel.de> > > > escreveu: > > > > > > > Hi, > > > > > > > > On 2020-03-30 15:07:40 -0300, Ranier Vilela wrote: > > > > > I'm not sure that the patch is 100% correct. > > > > > > > > This is *NOT* correct. > > > > > > > Anyway, the original source, still wrong. > > > What is the use of testing PageIsNew (page) twice in a row, if nothing > has > > > changed. > > > > Yea, that can be reduced. It's pretty harmless though. > > > > We used to require a cleanup lock (which requires dropping the lock, > > acquiring a cleanup lock - which allows for others to make the page be > > not empty) before acting on the empty page in vacuum. That's why > > PageIsNew() had to be checked again. Well, this is what the patch does, promove reduced and continue to check PageIsNew after unlock. regards, Ranier Vilela
[PATCH] remove condition always true (/src/backend/access/heap/vacuumlazy.c)
Hi, I'm not sure that the patch is 100% correct. But the fix is about expression about always true. But if this patch is correct, he fix one possible bug. The comment says: * Perform checking of FSM after releasing lock, the fsm is * approximate, after all. But this is not what the code does, apparently it checks before unlocking. best regards, Ranier Vilela remove_condition_always_true.patch Description: Binary data
[PATCH] Remove some reassigned values.
Hi, This patch remove reassigned values, with safety. Plancat.c, needs a more careful review. Best regards Ranier Vilela remove_reassigned_values.patch Description: Binary data
Re: Possible copy and past error? (\usr\backend\commands\analyze.c)
Em seg., 30 de mar. de 2020 às 06:06, Magnus Hagander escreveu: > On Sat, Mar 28, 2020 at 11:49 AM Ranier Vilela > wrote: > > > > Em sex., 27 de mar. de 2020 às 20:49, Tom Lane > escreveu: > >> > >> Ranier Vilela writes: > >> > Can someone check if there is a copy and paste error, at file: > >> > \usr\backend\commands\analyze.c, at lines 2225 and 2226? > >> > int num_mcv = stats->attr->attstattarget; > >> > int num_bins = stats->attr->attstattarget; > >> > >> No, that's intentional I believe. Those are independent variables > >> that just happen to start out with the same value. > > > > Neither you nor I can say with 100% certainty that the original author's > intention. > > Given that Tom is the original author, I think it's a lot more likely > that he knows what the original authors intention was. It's certainly > been a few years, so it probably isn't 100%, but the likelihood is > pretty good. > Of course, now we all know.. > > > >> > To silence this alert. > >> > >> If you have a tool that complains about that coding, I think the > >> tool needs a solid whack upside the head. There's nothing wrong > >> with the code, and it clearly expresses the intent, which the other > >> way doesn't. (Or in other words: it's the compiler's job to > >> optimize away the duplicate fetch. Not the programmer's.) > > > > I completely disagree. My tools have proven their worth, including > finding serious errors in the code, which fortunately have been fixed by > other committers. > > When issuing this alert, the tool does not value judgment regarding > performance or optimization, but it does an excellent job of finding > similar patterns in adjacent lines, and the only thing it asked for was to > be asked if this was really the case. original author's intention. > > All tools will give false positives. This simply seems one of those -- > it certainly could have been indicating a problem, but in this case it > didn't. > that's what you said, it could be a big problem, if it were the case of copy-past error. I do not consider it a false positive, since the tool did not claim it was a bug, she warned and asked to question. regards, Ranier Vilela
Re: Possible copy and past error? (\usr\backend\commands\analyze.c)
Em seg., 30 de mar. de 2020 às 05:16, Michael Paquier escreveu: > On Sat, Mar 28, 2020 at 07:48:22AM -0300, Ranier Vilela wrote: > > I completely disagree. My tools have proven their worth, including > finding > > serious errors in the code, which fortunately have been fixed by other > > committers. > > FWIW, I think that the rule to always take Coverity's reports with a > pinch of salt applies for any report. > I have certainly taken this advice seriously, since I have received all kinds of say, "words of discouragement". I understand perfectly that the list is very busy and perhaps the patience with mistakes is very little, but these attitudes do not help new people to work here. I don't get paid to work with PostgreSQL, so consideration and recognition are the only rewards for now. > > > When issuing this alert, the tool does not value judgment regarding > > performance or optimization, but it does an excellent job of finding > > similar patterns in adjacent lines, and the only thing it asked for was > to > > be asked if this was really the case. original author's intention. > > The code context matters a lot, but here let's leave this code alone. > There is nothing wrong with it. > That is the question. Looking only at the code, there is no way to know immediately, that there is nothing wrong. Not even a comment warning. That's what the tool asked for, ask if there's really nothing wrong. regards, Ranier Vilela
Re: [PATCH] Redudant initilization
Em dom., 29 de mar. de 2020 às 21:57, Kyotaro Horiguchi <
horikyota@gmail.com> escreveu:
> Hello.
>
> At Sat, 28 Mar 2020 19:04:00 -0300, Ranier Vilela
> wrote in
> > Hi,
> > This patch fixes some redundant initilization, that are safe to remove.
>
> > diff --git a/src/backend/access/gist/gistxlog.c
> b/src/backend/access/gist/gistxlog.c
> > index d3f3a7b803..ffaa2b1ab4 100644
> > --- a/src/backend/access/gist/gistxlog.c
> > +++ b/src/backend/access/gist/gistxlog.c
> > @@ -396,7 +396,7 @@ gistRedoPageReuse(XLogReaderState *record)
> > if (InHotStandby)
> > {
> > FullTransactionId latestRemovedFullXid =
> xlrec->latestRemovedFullXid;
> > - FullTransactionId nextFullXid =
> ReadNextFullTransactionId();
> > + FullTransactionId nextFullXid;
>
> I'd prefer to preserve this and remove the latter.
>
Ok.
> > diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
> > index 9d9e915979..795cf349eb 100644
> > --- a/src/backend/catalog/heap.c
> > +++ b/src/backend/catalog/heap.c
> > @@ -3396,7 +3396,7 @@ List *
> > heap_truncate_find_FKs(List *relationIds)
> > {
> > List *result = NIL;
> > - List *oids = list_copy(relationIds);
> > + List *oids;
>
> This was just a waste of memory, the fix looks fine.
>
> > diff --git a/src/backend/storage/smgr/md.c
> b/src/backend/storage/smgr/md.c
> > index c5b771c531..37fbeef841 100644
> > --- a/src/backend/storage/smgr/md.c
> > +++ b/src/backend/storage/smgr/md.c
> > @@ -730,9 +730,11 @@ mdwrite(SMgrRelation reln, ForkNumber forknum,
> BlockNumber blocknum,
> > BlockNumber
> > mdnblocks(SMgrRelation reln, ForkNumber forknum)
> > {
> > - MdfdVec*v = mdopenfork(reln, forknum, EXTENSION_FAIL);
> > + MdfdVec*v;
> > BlockNumber nblocks;
> > - BlockNumber segno = 0;
> > + BlockNumber segno;
> > +
> > +mdopenfork(reln, forknum, EXTENSION_FAIL);
> >
> > /* mdopen has opened the first segment */
> > Assert(reln->md_num_open_segs[forknum] > 0);
>
> It doesn't seems *to me* an issue.
>
Not a big deal, but the assignment of the variable v here is a small waste,
since it is again highlighted right after.
> > diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
> > index 8bb00abb6b..7a6a2ecbe9 100644
> > --- a/src/backend/utils/adt/json.c
> > +++ b/src/backend/utils/adt/json.c
> > @@ -990,7 +990,7 @@ catenate_stringinfo_string(StringInfo buffer, const
> char *addon)
> > Datum
> > json_build_object(PG_FUNCTION_ARGS)
> > {
> > - int nargs = PG_NARGS();
> > + int nargs;
>
> This part looks fine.
>
> > int i;
> > const char *sep = "";
> > StringInfo result;
> > @@ -998,6 +998,8 @@ json_build_object(PG_FUNCTION_ARGS)
> > bool *nulls;
> > Oid*types;
> >
> > +PG_NARGS();
> > +
> > /* fetch argument values to build the object */
> > nargs = extract_variadic_args(fcinfo, 0, false, &args, &types,
> &nulls);
>
> PG_NARGS() doesn't have a side-effect so no need to call independently.
>
Sorry, does that mean we can remove it completely?
>
> > diff --git a/src/backend/utils/mmgr/mcxt.c
> b/src/backend/utils/mmgr/mcxt.c
> > index 9e24fec72d..fb0e833b2d 100644
> > --- a/src/backend/utils/mmgr/mcxt.c
> > +++ b/src/backend/utils/mmgr/mcxt.c
> > @@ -475,7 +475,7 @@ MemoryContextMemAllocated(MemoryContext context,
> bool recurse)
> >
> > if (recurse)
> > {
> > - MemoryContext child = context->firstchild;
> > + MemoryContext child;
> >
> > for (child = context->firstchild;
> >child != NULL;
>
> This looks fine.
>
Thank you for the review and consideration.
best regards,
Ranier Vilela
