Re: public schema default ACL
On Wed, Aug 05, 2020 at 10:05:28PM -0700, Noah Misch wrote: > On Mon, Aug 03, 2020 at 07:46:02PM +0200, Peter Eisentraut wrote: > > The important things in my mind are that you keep an easy onboarding > > experience (you can do SQL things without having to create and unlock a > > bunch of things first) and that advanced users can do the things they want > > to do *somehow*. > > Makes sense. How do these options differ in ease of onboarding? In that > light, what option would you pick? Ping. Your statement above seems to suggest one of the options more than others, but I'd rather not assume you see it the same way.
Re: speed up unicode normalization quick check
On Thu, Oct 08, 2020 at 06:22:39PM -0400, John Naylor wrote: > Okay, thanks. And applied. I did some more micro benchmarking with the quick checks, and the numbers are cool, close to what you mentioned for the quick checks of both NFC and NFKC. Just wondering about something in the same area, did you look at the decomposition table? -- Michael signature.asc Description: PGP signature
Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston < david.g.johns...@gmail.com> escreveu: > On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela wrote: > >> The problem is not only in nodeIncrementalSort.c, but in several others >> too, where people are using TupIsNull with ExecCopySlot. >> I would call this a design flaw. >> If (TupIsNull) >> ExecCopySlot >> >> The callers, think they are using TupIsNotNullAndEmpty. >> If (TupIsNotNullAndEmpty) >> ExecCopySlot >> > > IMO both names are problematic, too data value centric, not semantic. > TupIsValid for the name and negating the existing tests would help to at > least clear that part up. Then, things operating on invalid tuples would > be expected to know about both representations. In the case of > ExecCopySlot there is nothing it can do with a null representation of an > invalid tuple so it would have to fail if presented one. An assertion > seems sufficient. > IHMO, assertion it is not the solution. Steven suggested looking for some NULL pointer font above the calls. I say that it is not necessary, there is no NULL pointer. Whoever guarantees this is the combination, which for me is an assertion. If (TupIsNull) ExecCopySlot It works as a subject, but in release mode. It is the equivalent of: If (TupIsNull) Abort The only problem for me is that we are running this assertion on the clients' machines. regards, Ranier Vilela >
Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
Noah Misch writes: > The first attachment fixes the matter you've reported. While confirming that, > I observed that gcc builds don't even use the 64-bit code in arch-ppc.h. > Oops. The second attachment fixes that. I reviewed these, and tested the first one on a nearby Apple machine. (I lack access to 64-bit PPC, so I can't actually test the second.) They look fine, and I confirmed by examining asm output that even the rather-old-now gcc version that Apple last shipped for PPC does the right thing with the conditionals. > I plan not to back-patch either of these. Hmm, I'd argue for a back-patch. The issue of modern compilers warning about the incorrect code will apply to all supported branches. Moreover, even if we don't use these code paths today, who's to say that someone won't back-patch a bug fix that requires them? I do not think it's unreasonable to expect these functions to work well in all branches that have them. regards, tom lane
Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
On Sun, Oct 11, 2020 at 3:31 AM Ranier Vilela wrote: > Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston < > david.g.johns...@gmail.com> escreveu: > >> On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela wrote: >> >>> The problem is not only in nodeIncrementalSort.c, but in several others >>> too, where people are using TupIsNull with ExecCopySlot. >>> I would call this a design flaw. >>> If (TupIsNull) >>> ExecCopySlot >>> >>> The callers, think they are using TupIsNotNullAndEmpty. >>> If (TupIsNotNullAndEmpty) >>> ExecCopySlot >>> >> >> IMO both names are problematic, too data value centric, not semantic. >> TupIsValid for the name and negating the existing tests would help to at >> least clear that part up. Then, things operating on invalid tuples would >> be expected to know about both representations. In the case of >> ExecCopySlot there is nothing it can do with a null representation of an >> invalid tuple so it would have to fail if presented one. An assertion >> seems sufficient. >> > IHMO, assertion it is not the solution. > > Steven suggested looking for some NULL pointer font above the calls. > I say that it is not necessary, there is no NULL pointer. > Whoever guarantees this is the combination, which for me is an assertion. > > If (TupIsNull) >ExecCopySlot > > It works as a subject, but in release mode. > It is the equivalent of: > > If (TupIsNull) >Abort > > The only problem for me is that we are running this assertion on the > clients' machines. > > I cannot make heads nor tails of what you are trying to communicate here. I'll agree that TupIsNull isn't the most descriptive choice of name, and is probably being abused throughout the code base, but the overall intent and existing flow seems fine. My only goal would be to make it a bit easier for unfamiliar coders to pick up on the coding pattern and assumptions and make coding errors there more obvious. Renaming and/or an assertion fits that goal. Breaking the current abstraction level doesn't seem desirable. David J.
Re: [PATCH] Add `truncate` option to subscription commands
On Fri, 9 Oct 2020 at 15:54, David Christensen wrote: > > Enclosed find a patch to add a “truncate” option to subscription commands. > > When adding new tables to a subscription (either via `CREATE SUBSCRIPTION` > or `REFRESH PUBLICATION`), tables on the target which are being newly > subscribed will be truncated before the data copy step. This saves > explicit coordination of a manual `TRUNCATE` on the target tables and > allows the results of the initial data sync to be the same as on the > publisher at the time of sync. > > To preserve compatibility with existing behavior, the default value for > this parameter is `false`. > > Truncate will fail for tables whose foreign keys refer to it. If such a feature cannot handle foreign keys, the usefulness will be restricted. Regards, -- Euler Taveira http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
Re: Tom Lane > > I plan not to back-patch either of these. > > Hmm, I'd argue for a back-patch. The issue of modern compilers > warning about the incorrect code will apply to all supported branches. > Moreover, even if we don't use these code paths today, who's to say > that someone won't back-patch a bug fix that requires them? I do not > think it's unreasonable to expect these functions to work well in > all branches that have them. Or remove them. (But fixing seems better.) Christoph
Re: [PATCH] Add `truncate` option to subscription commands
> On Oct 11, 2020, at 1:14 PM, Euler Taveira > wrote: > > >> On Fri, 9 Oct 2020 at 15:54, David Christensen wrote: > >> >> Enclosed find a patch to add a “truncate” option to subscription commands. >> >> When adding new tables to a subscription (either via `CREATE SUBSCRIPTION` >> or `REFRESH PUBLICATION`), tables on the target which are being newly >> subscribed will be truncated before the data copy step. This saves explicit >> coordination of a manual `TRUNCATE` on the target tables and allows the >> results of the initial data sync to be the same as on the publisher at the >> time of sync. >> >> To preserve compatibility with existing behavior, the default value for this >> parameter is `false`. >> > > Truncate will fail for tables whose foreign keys refer to it. If such a > feature cannot handle foreign keys, the usefulness will be restricted. This is true for existing “truncate” with FKs, so doesn’t seem to be any different to me. Hypothetically if you checked all new tables and could verify if there were FK cycles only already in the new tables being added then “truncate cascade” would be fine. Arguably if they had existing tables that were part of an FK that wasn’t fully replicated they were already operating brokenly. But you would definitely want to avoid “truncate cascade” if the FK target tables were already in the publication, unless we were willing to re-sync the other tables that would be truncated. David
Re: BUG #15858: could not stat file - over 4GB
On Sat, Oct 10, 2020 at 08:34:48PM -0400, Tom Lane wrote:
> Nah, I fixed that hours ago (961e07b8c). jacana must not have run again
> yet.
Indeed, thanks. I have missed one sync here.
+ hFile = CreateFile(name,
+ GENERIC_READ,
+ (FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE),
+ &sa,
+ OPEN_EXISTING,
+ (FILE_FLAG_NO_BUFFERING | FILE_FLAG_BACKUP_SEMANTICS |
+ FILE_FLAG_OVERLAPPED),
+ NULL);
+ if (hFile == INVALID_HANDLE_VALUE)
+ {
+ CloseHandle(hFile);
+ errno = ENOENT;
+ return -1;
+ }
Why are we forcing errno=ENOENT here? Wouldn't it be correct to use
_dosmaperr(GetLastError()) to get the correct errno? This code would
for example consider as non-existing a file even if we fail getting it
because of ERROR_SHARING_VIOLATION, which should map to EACCES. This
case can happen with virus scanners taking a non-share handle on files
being looked at in parallel of this code path.
--
Michael
signature.asc
Description: PGP signature
Re: Wrong statistics for size of XLOG_SWITCH during pg_waldump.
On Sat, Oct 10, 2020 at 09:50:02AM +0800, movead...@highgo.ca wrote:
>> I think that the length of the XLOG_SWITCH record is no other than 24
>> bytes. Just adding the padding? garbage bytes to that length doesn't
>> seem the right thing to me.
>
> Here's the lookes:
> rmgr: XLOGlen (rec/tot): 24/24, tx: 0, lsn:
> 0/03D8, prev 0/0360, desc: SWITCH, trailing-bytes: 16776936
static void
-XLogDumpRecordLen(XLogReaderState *record, uint32 *rec_len, uint32 *fpi_len)
+XLogDumpRecordLen(XLogReaderState *record, uint32 *rec_len, uint32 *fpi_len,
uint32 *junk_len)
{
If you wish to add more information about a XLOG_SWITCH record, I
don't think that changing the signature of XLogDumpRecordLen() is
adapted because the record length of this record is defined as
Horiguchi-san mentioned upthread, and the meaning of junk_len is
confusing here. It seems to me that any extra information should be
added to xlog_desc() where there should be an extra code path for
(info == XLOG_SWITCH). XLogReaderState should have all the
information you are lookng for.
--
Michael
signature.asc
Description: PGP signature
Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
On Sun, Oct 11, 2020 at 08:35:13PM +0200, Christoph Berg wrote: > Re: Tom Lane >> Hmm, I'd argue for a back-patch. The issue of modern compilers >> warning about the incorrect code will apply to all supported branches. >> Moreover, even if we don't use these code paths today, who's to say >> that someone won't back-patch a bug fix that requires them? I do not >> think it's unreasonable to expect these functions to work well in >> all branches that have them. > > Or remove them. (But fixing seems better.) The patch is not that invasive, so just fixing back-branches sounds like a good idea to me. My 2c. -- Michael signature.asc Description: PGP signature
Re: Parallel INSERT (INTO ... SELECT ...)
On Sat, Oct 10, 2020 at 3:32 PM Amit Kapila wrote: > > > OK, for the minimal patch, just allowing INSERT with parallel SELECT, > > you're right, neither of those additional "commandType == CMD_SELECT" > > checks are needed, so I'll remove them. > > >various > Okay, that makes sense. > For the minimal patch (just allowing INSERT with parallel SELECT), there are issues with parallel-mode and various parallel-mode-related checks in the code. Initially, I thought it was only a couple of XID-related checks (which could perhaps just be tweaked to check for IsParallelWorker() instead, as you suggested), but I now realise that there are a lot more cases. This stems from the fact that just having a parallel SELECT (as part of non-parallel INSERT) causes parallel-mode to be set for the WHOLE plan. I'm not sure why parallel-mode is set globally like this, for the whole plan. Couldn't it just be set for the scope of Gather/GatherMerge? Otherwise, errors from these checks seem to be misleading when outside the scope of Gather/GatherMerge, as technically they are not occurring within the scope of parallel-leader and parallel-worker(s). The global parallel-mode wouldn't have been an issue before, because up to now INSERT has never had underlying parallel operations. For example, when running the tests under "force_parallel_mode=regress", the test failures show that there are a lot more cases affected: "cannot assign TransactionIds during a parallel operation" "cannot assign XIDs during a parallel operation" "cannot start commands during a parallel operation" "cannot modify commandid in active snapshot during a parallel operation" "cannot execute nextval() during a parallel operation" "cannot execute INSERT during a parallel operation" "cannot execute ANALYZE during a parallel operation "cannot update tuples during a parallel operation" (and there are more not currently detected by the tests, found by searching the code). As an example, with the minimal patch applied, if you had a trigger on INSERT that, say, attempted a table creation or UPDATE/DELETE, and you ran an "INSERT INTO ... SELECT...", it would treat the trigger operations as being attempted in parallel-mode, and so an error would result. Let me know your thoughts on how to deal with these issues. Can you see a problem with only having parallel-mode set for scope of Gather/GatherMerge, or do you have some other idea? Regards, Greg Nancarrow Fujitsu Australia
Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
Em dom., 11 de out. de 2020 às 14:53, David G. Johnston < david.g.johns...@gmail.com> escreveu: > On Sun, Oct 11, 2020 at 3:31 AM Ranier Vilela wrote: > >> Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston < >> david.g.johns...@gmail.com> escreveu: >> >>> On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela >>> wrote: >>> The problem is not only in nodeIncrementalSort.c, but in several others too, where people are using TupIsNull with ExecCopySlot. I would call this a design flaw. If (TupIsNull) ExecCopySlot, The callers, think they are using TupIsNotNullAndEmpty. If (TupIsNotNullAndEmpty) ExecCopySlot >>> >>> IMO both names are problematic, too data value centric, not semantic. >>> TupIsValid for the name and negating the existing tests would help to at >>> least clear that part up. Then, things operating on invalid tuples would >>> be expected to know about both representations. In the case of >>> ExecCopySlot there is nothing it can do with a null representation of an >>> invalid tuple so it would have to fail if presented one. An assertion >>> seems sufficient. >>> >> IHMO, assertion it is not the solution. >> >> Steven suggested looking for some NULL pointer font above the calls. >> I say that it is not necessary, there is no NULL pointer. >> Whoever guarantees this is the combination, which for me is an assertion. >> >> If (TupIsNull) >>ExecCopySlot >> >> It works as a subject, but in release mode. >> It is the equivalent of: >> >> If (TupIsNull) >>Abort >> >> The only problem for me is that we are running this assertion on the >> clients' machines. >> >> > I cannot make heads nor tails of what you are trying to communicate here. > Ok. I will try to explain. 1. TupIsNull in fact it should be called: TupIsNullOrEmpty 2. Only Rename TupIsNull to example TupIsNullOrEmpty, improves, but it is not the complete solution. 3. The combination: if (TupIsNull(node->group_pivot)) ExecCopySlot(node->group_pivot, node->transfer_tuple); for me it acts partly as if it were an assertion, but at runtime. If node->group_pivot is NULL, ExecCopySlot crashes, like an assertion. 4. As it has been running for a while, without any complaints, probably the callers have already guaranteed that node-> group_pivot is not NULL 5. We can remove the first part of the macro and rename: TupIsNull to SlotEmpty 6. With SlotEmpty macro, each TupIsNull needs to be carefully changed. if (SlotEmpty(node->group_pivot)) ExecCopySlot(node->group_pivot, node->transfer_tuple); > I'll agree that TupIsNull isn't the most descriptive choice of name, and > is probably being abused throughout the code base, but the overall intent > and existing flow seems fine. My only goal would be to make it a bit > easier for unfamiliar coders to pick up on the coding pattern and > assumptions and make coding errors there more obvious. Renaming and/or an > assertion fits that goal. Breaking the current abstraction level doesn't > seem desirable. > If only rename TupIsNull to TupIsNullOrEmpty: 1. Why continue testing a pointer against NULL and call ExecCopySlot and crash at runtime. 2. Most likely, the pointer is not NULL, since it has already been well tested. 3. The only thing that can be done, after TupIsNullOrEmpty, is return or fail, anything else needs to be tested again. I think that current abstraction is broken. regards, Ranier Vilela
Re: Parallel INSERT (INTO ... SELECT ...)
On Sun, Oct 11, 2020 at 1:05 PM Amit Kapila wrote: > > On Sat, Oct 10, 2020 at 5:25 PM Amit Kapila wrote: > > > > 8. You have made changes related to trigger execution for Gather node, > > don't we need similar changes for GatherMerge node? > > > .. > > > > 10. Don't we need a change similar to cost_gather in > > cost_gather_merge? It seems you have made only partial changes for > > GatherMerge node. > > > > Please ignore these two comments as we can't push Insert to workers > when GatherMerge is involved as a leader backend does the final phase > (merge the results by workers). So, we can only push the Select part > of the statement. > Precisely, that's why I didn't make those changes for GatherMerge. Regards, Greg Nancarrow Fujitsu Australia
RE: Transactions involving multiple postgres foreign servers, take 2
From: Masahiko Sawada > I also doubt how useful the per-foreign-server timeout setting you > mentioned before. For example, suppose the transaction involves with > three foreign servers that have different timeout setting, what if the > backend failed to commit on the first one of the server due to > timeout? Does it attempt to commit on the other two servers? Or does > it give up and return the control to the client? In the former case, > what if the backend failed again on one of the other two servers due > to timeout? The backend might end up waiting for all timeouts and in > practice the user is not aware of how many servers are involved with > the transaction, for example in a sharding. So It seems to be hard to > predict the total timeout. In the latter case, the backend might > succeed to commit on the other two nodes. Also, the timeout setting of > the first foreign server virtually is used as the whole foreign > transaction resolution timeout. However, the user cannot control the > order of resolution. So again it seems to be hard for the user to > predict the timeout. So If we have a timeout mechanism, I think it's > better if the user can control the timeout for each transaction. > Probably the same is true for the retry. I agree that the user can control the timeout per transaction, not per FDW. I was just not sure if the Postgres core can define the timeout parameter and the FDWs can follow its setting. However, JTA defines a transaction timeout API (not commit timeout, though), and each RM can choose to implement them. So I think we can define the parameter and/or routines for the timeout in core likewise. -- public interface javax.transaction.xa.XAResource int getTransactionTimeout() throws XAException This method returns the transaction timeout value set for this XAResourceinstance. If XAResource. setTransactionTimeout was not use prior to invoking this method, the return value is the default timeout set for the resource manager; otherwise, the value used in the previous setTransactionTimeoutcall is returned. Throws: XAException An error has occurred. Possible exception values are: XAER_RMERR, XAER_RMFAIL. Returns: The transaction timeout values in seconds. boolean setTransactionTimeout(int seconds) throws XAException This method sets the transaction timeout value for this XAResourceinstance. Once set, this timeout value is effective until setTransactionTimeoutis invoked again with a different value. To reset the timeout value to the default value used by the resource manager, set the value to zero. If the timeout operation is performed successfully, the method returns true; otherwise false. If a resource manager does not support transaction timeout value to be set explicitly, this method returns false. Parameters: seconds An positive integer specifying the timout value in seconds. Zero resets the transaction timeout value to the default one used by the resource manager. A negative value results in XAException to be thrown with XAER_INVAL error code. Returns: true if transaction timeout value is set successfully; otherwise false. Throws: XAException An error has occurred. Possible exception values are: XAER_RMERR, XAER_RMFAIL, or XAER_INVAL. -- > For example in postgres_fdw, it executes a SQL in asynchronous manner > using by PQsendQuery(), PQconsumeInput() and PQgetResult() and so on > (see do_sql_command() and pgfdw_get_result()). Therefore it the user > pressed ctl-C, the remote query would be canceled and raise an ERROR. Yeah, as I replied to Horiguchi-san, postgres_fdw can cancel queries. But postgres_fdw is not ready to cancel connection establishment, is it? At present, the user needs to set connect_timeout parameter on the foreign server to a reasonable short time so that it can respond quickly to cancellation requests. Alternately, we can modify postgres_fdw to use libpq's asynchronous connect functions. Another issue is that the Postgres manual does not stipulate anything about cancellation of FDW processing. That's why I said that the current FDW does not support cancellation in general. Of course, I think we can stipulate the ability to cancel processing in the FDW interface. Regards Takayuki Tsunakawa
Re: [HACKERS] Runtime Partition Pruning
Hi Ashutosh: On Thu, Oct 8, 2020 at 7:25 PM Ashutosh Bapat wrote: > On Wed, Oct 7, 2020 at 7:00 PM Andy Fan wrote: > > > > > > > > On Wed, Oct 7, 2020 at 5:05 PM Andy Fan > wrote: > >> > >> > >> > >> On Sun, Oct 4, 2020 at 3:10 PM Andy Fan > wrote: > > > > Now, in my experience, the current system for custom plans vs. generic > plans doesn't approach the problem in this way at all, and in my > experience that results in some pretty terrible behavior. It will do > things like form a custom plan every time because the estimated cost > of the custom plan is lower than the estimated cost of the generic > plan even though the two plans are structurally identical; only the > estimates differ. It will waste gobs of CPU cycles by replanning a > primary key lookup 5 times just on the off chance that a lookup on the > primary key index isn't the best option. But this patch isn't going > to fix any of that. The best we can probably do is try to adjust the > costing for Append paths in some way that reflects the costs and > benefits of pruning. I'm tentatively in favor of trying to do > something modest in that area, but I don't have a detailed proposal. > > >>> > >>> I just realized this issue recently and reported it at [1], then Amit > pointed > >>> me to this issue being discussed here, so I would like to continue > this topic > >>> here. > >>> > >>> I think we can split the issue into 2 issues. One is the partition > prune in initial > >>> partition prune, which maybe happen in custom plan case only and caused > >>> the above issue. The other one happens in the "Run-Time" partition > prune, > >>> I admit that is an important issue to resolve as well, but looks > harder. So I > >>> think we can fix the first one at first. > >>> > >>> ... When we count for the cost of a > >>> generic plan, we can reduce the cost based on such information. > >> > >> > >> This way doesn't work since after the initial partition prune, not > only the > >> cost of the Append node should be reduced, the cost of other plans > should > >> be reduced as well [1] > >> > >> However I think if we can use partition prune information from a custom > plan > >> at the cost_append_path stage, it looks the issue can be fixed. If > so, the idea > >> is similar to David's idea in [2], however Robert didn't agree with > this[2]. > >> Can anyone elaborate this objection? for a partkey > $1 or BETWEEN > cases, > >> some real results from the past are probably better than some hard-coded > >> assumptions IMO. > > > > > > I can understand Robert's idea now, he intended to resolve both the > > "initial-partition-prune" case and "runtime partition prune" case while > I always think > > about the former case (Amit reminded me about that at the beginning, but > I just > > wake up right now..) > > > > With the Robert's idea, I think we can do some hack at > create_append_path, > > we can figure out the pruneinfo (it was done at create_append_plan now) > at that > > stage, and then did a fix_append_path with such pruneinfo. We need to > fix not > > only the cost of AppendPath, but also rows of AppendPath, For example: > > SELECT .. FROM t1, t2, p where t1.a = p.partkey and t1.b = t2.b, if the > > plan is: > > Nest Loop > >Nest Loop > > t1 > > Append (p) > >t2 > > > > Then the rows of Append (p) will be very important. > > > > For this idea, how to estimate how many partitions/rows can be pruned > is a key > > part. Robert said "I was thinking, rather, that if we know for example > that > > we've doing pruning on partition_column = $1, then we know that only > > one partition will match. That's probably a common case. If we've > > got partition_column > $1, we could assume that, say, 75% of the > > partitions would match. partition_column BETWEEN $1 and $2 is > > probably a bit more selective, so maybe we assume 50% of the > > partitions would match.". I think we can't say the 75% or 50% is a good > > number, but the keypoint may be "partition_column = $1" is a common > > case. And for the others case, we probably don't make it worse. > > > > I think we need to do something here, any thoughts? Personally I'm more > > like this idea now. > > Yes, I think we have to do something on those lines. But I am > wondering why our stats machinery is failing to do that already. For > equality I think it's straight forward. But even for other operators > we should use the statistics from the partitioned table to estimate > the selectivity and then adjust number of scanned partitions = (number > of partitions * fraction of rows scanned). That might be better than > 50% or 75%. > > Sorry for the late reply! Suppose we have partition defined like this: p - p1 - p2 When you talk about "the statistics from the partitioned table", do you mean the statistics from p or p1/p2? I just confirmed there is no statistics for p (at least pg_class.re
Re: [PATCH] Keeps tracking the uniqueness with UniqueKey
On Thu, Oct 8, 2020 at 12:12 PM Hou, Zhijie
wrote:
> Hi
>
> I have a look over this patch and find some typos in 0002.
>
> 1.Some typos about unique:
> There are some spelling mistakes about "unique" in code comments and
> README.
> Such as: "+However we define the UnqiueKey as below."
>
> 2.function name about initililze_uniquecontext_for_joinrel:
> May be it should be initialize_ uniquecontext_for_joinrel.
>
> 3.some typos in comment:
> +* baserelation's basicrestrictinfo. so it must be
> in ON clauses.
>
> I think it shoule be " basicrestrictinfo " => "baserestrictinfo".
>
>
> Besides, I think list_copy can be used to simplify the following code.
> (But It seems the type of expr is still in discussion, so this may has no
> impact )
> + List*exprs = NIL;
> ...
> + foreach(lc, unionrel->reltarget->exprs)
> + {
> + exprs = lappend(exprs, lfirst(lc));
> + }
>
> Best regards,
>
>
>
Thank you zhijie, I will fix them in next version.
--
Best Regards
Andy Fan
Re: [PATCH] Keeps tracking the uniqueness with UniqueKey
On Thu, Oct 8, 2020 at 6:39 PM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > On Thu, Oct 08, 2020 at 09:34:51AM +0800, Andy Fan wrote: > > > > > Other than that I wanted to ask what are the plans to proceed with this > > > patch? It's been a while since the question was raised in which format > > > to keep unique key expressions, and as far as I can see no detailed > > > suggestions or patch changes were proposed as a follow up. Obviously I > > > would love to see the first two preparation patches committed to avoid > > > dependencies between patches, and want to suggest an incremental > > > approach with simple format for start (what we have right now) with the > > > idea how to extend it in the future to cover more cases. > > > > > > > I think the hardest part of this series is commit 2, it probably needs > > lots of > > dedicated time to review which would be the hardest part for the > reviewers. > > I don't have a good suggestion, however. > > Sure, and I would review the patch as well. Thank you very much! > But as far as I understand > the main issue is "how to store uniquekey expressions", and as long as > it is not decided, no additional review will move the patch forward I > guess. > I don't think so:) The patch may have other issues as well. For example, logic error or duplicated code or cases needing improvement and so on. -- Best Regards Andy Fan
Re: Parallel INSERT (INTO ... SELECT ...)
On Sun, Oct 11, 2020 at 1:39 PM Thomas Munro wrote: > > Yeah, I think this is trying to fix the problem too late. Instead, we > should fix the incorrect row estimates so we don't have to fudge it > later like that. For example, this should be estimating rows=0: > > postgres=# explain analyze insert into s select * from t t1 join t t2 using > (i); > ... > Insert on s (cost=30839.08..70744.45 rows=1000226 width=4) (actual > time=2940.560..2940.562 rows=0 loops=1) > > I think that should be done with something like this: > > --- a/src/backend/optimizer/util/pathnode.c > +++ b/src/backend/optimizer/util/pathnode.c > @@ -3583,16 +3583,11 @@ create_modifytable_path(PlannerInfo *root, > RelOptInfo *rel, > if (lc == list_head(subpaths)) /* first node? */ > pathnode->path.startup_cost = subpath->startup_cost; > pathnode->path.total_cost += subpath->total_cost; > - pathnode->path.rows += subpath->rows; > + if (returningLists != NIL) > + pathnode->path.rows += subpath->rows; > total_size += subpath->pathtarget->width * subpath->rows; > } > > - /* > -* Set width to the average width of the subpath outputs. XXX this is > -* totally wrong: we should report zero if no RETURNING, else an > average > -* of the RETURNING tlist widths. But it's what happened > historically, > -* and improving it is a task for another day. > -*/ > if (pathnode->path.rows > 0) > total_size /= pathnode->path.rows; > pathnode->path.pathtarget->width = rint(total_size); Agree, thanks (bug in existing Postgres code, right?) Regards, Greg Nancarrow Fujitsu Australia
Re: Possible NULL dereferencing null pointer (src/backend/executor/nodeIncrementalSort.c)
On Sun, Oct 11, 2020 at 6:27 PM Ranier Vilela wrote: > Em dom., 11 de out. de 2020 às 14:53, David G. Johnston < > david.g.johns...@gmail.com> escreveu: > >> On Sun, Oct 11, 2020 at 3:31 AM Ranier Vilela >> wrote: >> >>> Em sáb., 10 de out. de 2020 às 00:11, David G. Johnston < >>> david.g.johns...@gmail.com> escreveu: >>> On Fri, Oct 9, 2020 at 6:41 PM Ranier Vilela wrote: > The problem is not only in nodeIncrementalSort.c, but in several > others too, where people are using TupIsNull with ExecCopySlot. > I would call this a design flaw. > If (TupIsNull) > ExecCopySlot, > > The callers, think they are using TupIsNotNullAndEmpty. > If (TupIsNotNullAndEmpty) > ExecCopySlot > IMO both names are problematic, too data value centric, not semantic. TupIsValid for the name and negating the existing tests would help to at least clear that part up. Then, things operating on invalid tuples would be expected to know about both representations. In the case of ExecCopySlot there is nothing it can do with a null representation of an invalid tuple so it would have to fail if presented one. An assertion seems sufficient. >>> IHMO, assertion it is not the solution. >>> >>> Steven suggested looking for some NULL pointer font above the calls. >>> I say that it is not necessary, there is no NULL pointer. >>> Whoever guarantees this is the combination, which for me is an assertion. >>> >>> If (TupIsNull) >>>ExecCopySlot >>> >>> It works as a subject, but in release mode. >>> It is the equivalent of: >>> >>> If (TupIsNull) >>>Abort >>> >>> The only problem for me is that we are running this assertion on the >>> clients' machines. >>> >>> >> I cannot make heads nor tails of what you are trying to communicate here. >> > Ok. I will try to explain. > > 1. TupIsNull in fact it should be called: TupIsNullOrEmpty > 2. Only Rename TupIsNull to example TupIsNullOrEmpty, improves, but it is > not the complete solution. > 3. The combination: > if (TupIsNull(node->group_pivot)) > ExecCopySlot(node->group_pivot, node->transfer_tuple); > for me it acts partly as if it were an assertion, but at runtime. > If node->group_pivot is NULL, ExecCopySlot crashes, like an assertion. > Ok, but for me it's not an assertion, it's a higher-level check that the variable that is expected to hold data on subsequent loops is, at the beginning of the loop, uninitialized. TupIsUninitialized comes to mind as better reflecting that fact. 4. As it has been running for a while, without any complaints, probably the > callers have already guaranteed that node-> group_pivot is not NULL > 5. We can remove the first part of the macro and rename: TupIsNull to > SlotEmpty > 6. With SlotEmpty macro, each TupIsNull needs to be carefully changed. > if (SlotEmpty(node->group_pivot)) > ExecCopySlot(node->group_pivot, node->transfer_tuple); > I don't have a problem with introducing a SlotEmpty macro, and agree that when it is followed by "ExecCopySlot" it is an meaningful improvement (the blurring of the lines between a slot and its pointed-to-tuple bothers me as I get my first exposure this to code). > > >> I'll agree that TupIsNull isn't the most descriptive choice of name, and >> is probably being abused throughout the code base, but the overall intent >> and existing flow seems fine. My only goal would be to make it a bit >> easier for unfamiliar coders to pick up on the coding pattern and >> assumptions and make coding errors there more obvious. Renaming and/or an >> assertion fits that goal. Breaking the current abstraction level doesn't >> seem desirable. >> > > If only rename TupIsNull to TupIsNullOrEmpty: > > 1. Why continue testing a pointer against NULL and call ExecCopySlot and > crash at runtime. > 2. Most likely, the pointer is not NULL, since it has already been well > tested. > 3. The only thing that can be done, after TupIsNullOrEmpty, is return or > fail, anything else needs to be tested again. > > I think that current abstraction is broken. > I'm willing to agree that the abstraction is broken even if the end result of its use, in the existing codebase, hasn't resulted in any known bugs (again, the null pointer dereferencing seems like it should be picked up during routine testing). That said, there are multiple solutions here that would improve matters in varying degrees each having a proportional effort and risk profile in writing a patch (including the status-quo option). For me, while I see room for improvement here, my total lack of actually writing code using these interfaces means I defer to Tom Lane's final two conclusions in his last email regarding how productive this line of work would be. I also read that to mean if there was a complete and thorough patch submitted it would be given a fair look. I would hope so since there is a meaningful decision to make with regards to making changes purely to bene
Re: [PATCH] Add `truncate` option to subscription commands
On Mon, Oct 12, 2020 at 3:44 AM David Christensen wrote: > > > On Oct 11, 2020, at 1:14 PM, Euler Taveira > wrote: > > > On Fri, 9 Oct 2020 at 15:54, David Christensen wrote: >> >> >> Enclosed find a patch to add a “truncate” option to subscription commands. >> >> When adding new tables to a subscription (either via `CREATE SUBSCRIPTION` >> or `REFRESH PUBLICATION`), tables on the target which are being newly >> subscribed will be truncated before the data copy step. This saves explicit >> coordination of a manual `TRUNCATE` on the target tables and allows the >> results of the initial data sync to be the same as on the publisher at the >> time of sync. >> >> To preserve compatibility with existing behavior, the default value for this >> parameter is `false`. >> > > Truncate will fail for tables whose foreign keys refer to it. If such a > feature cannot handle foreign keys, the usefulness will be restricted. > > > This is true for existing “truncate” with FKs, so doesn’t seem to be any > different to me. > What would happen if there are multiple tables and truncate on only one of them failed due to FK check? Does it give an error in such a case, if so will the other tables be truncated? > Hypothetically if you checked all new tables and could verify if there were > FK cycles only already in the new tables being added then “truncate cascade” > would be fine. Arguably if they had existing tables that were part of an FK > that wasn’t fully replicated they were already operating brokenly. > I think if both PK_table and FK_table are part of the same subscription then there should be a problem as both them first get truncated? If they are part of a different subscription (or if they are not subscribed due to whatever reason) then probably we need to deal such cases carefully. -- With Regards, Amit Kapila.
Re: Parallel INSERT (INTO ... SELECT ...)
On Mon, Oct 12, 2020 at 3:42 PM Greg Nancarrow wrote:
> On Sun, Oct 11, 2020 at 1:39 PM Thomas Munro wrote:
> > pathnode->path.total_cost += subpath->total_cost;
> > - pathnode->path.rows += subpath->rows;
> > + if (returningLists != NIL)
> > + pathnode->path.rows += subpath->rows;
> > total_size += subpath->pathtarget->width * subpath->rows;
> > }
Erm, except the condition should of course cover total_size too.
> Agree, thanks (bug in existing Postgres code, right?)
Yeah, I think we should go ahead and fix that up front. Here's a
version with a commit message.
From d92c45e6c1fa5ed118195de149d8fb833ea008ef Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 12 Oct 2020 15:49:27 +1300
Subject: [PATCH] Fix row estimate for ModifyTable paths.
Historically, we estimated that a ModifyTable node would emit the same
number of rows as it writes. That's only true if there is a RETURNING
clause, otherwise the real number is zero. It didn't matter much
before, but proposed patches to allow for parallel writes revealed that
such queries would look unduly expensive if we continued to charge for a
phantom RETURNING clause.
Correct the estimates in master only. There doesn't seem to be much
point in back-patching the change for now.
Reviewed-by: Greg Nancarrow
Discussion: https://postgr.es/m/CAJcOf-cXnB5cnMKqWEp2E2z7Mvcd04iLVmV%3DqpFJrR3AcrTS3g%40mail.gmail.com
---
src/backend/optimizer/util/pathnode.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index c1fc866cbf..d718a4adba 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3583,16 +3583,13 @@ create_modifytable_path(PlannerInfo *root, RelOptInfo *rel,
if (lc == list_head(subpaths)) /* first node? */
pathnode->path.startup_cost = subpath->startup_cost;
pathnode->path.total_cost += subpath->total_cost;
- pathnode->path.rows += subpath->rows;
- total_size += subpath->pathtarget->width * subpath->rows;
+ if (returningLists != NIL)
+ {
+ pathnode->path.rows += subpath->rows;
+ total_size += subpath->pathtarget->width * subpath->rows;
+ }
}
- /*
- * Set width to the average width of the subpath outputs. XXX this is
- * totally wrong: we should report zero if no RETURNING, else an average
- * of the RETURNING tlist widths. But it's what happened historically,
- * and improving it is a task for another day.
- */
if (pathnode->path.rows > 0)
total_size /= pathnode->path.rows;
pathnode->path.pathtarget->width = rint(total_size);
--
2.20.1
Re: BUG #15858: could not stat file - over 4GB
Michael Paquier writes: > Why are we forcing errno=ENOENT here? Wouldn't it be correct to use > _dosmaperr(GetLastError()) to get the correct errno? Fair question. Juan, was there some good reason not to look at GetLastError() in this step? regards, tom lane
Re: Parallel INSERT (INTO ... SELECT ...)
On Mon, Oct 12, 2020 at 6:51 AM Greg Nancarrow wrote: > > On Sat, Oct 10, 2020 at 3:32 PM Amit Kapila wrote: > > > > > OK, for the minimal patch, just allowing INSERT with parallel SELECT, > > > you're right, neither of those additional "commandType == CMD_SELECT" > > > checks are needed, so I'll remove them. > > > > >various > > Okay, that makes sense. > > > > For the minimal patch (just allowing INSERT with parallel SELECT), > there are issues with parallel-mode and various parallel-mode-related > checks in the code. > Initially, I thought it was only a couple of XID-related checks (which > could perhaps just be tweaked to check for IsParallelWorker() instead, > as you suggested), but I now realise that there are a lot more cases. > This stems from the fact that just having a parallel SELECT (as part > of non-parallel INSERT) causes parallel-mode to be set for the WHOLE > plan. I'm not sure why parallel-mode is set globally like this, for > the whole plan. Couldn't it just be set for the scope of > Gather/GatherMerge? Otherwise, errors from these checks seem to be > misleading when outside the scope of Gather/GatherMerge, as > technically they are not occurring within the scope of parallel-leader > and parallel-worker(s). The global parallel-mode wouldn't have been an > issue before, because up to now INSERT has never had underlying > parallel operations. > That is right but there is another operation which works like that. For ex. a statement like "create table test_new As select * from test_parallel where c1 < 1000;" will use parallel select but the write operation will be performed in a leader. I agree that the code flow of Insert is different so we will have a different set of challenges in that case but to make it work there shouldn't be any fundamental problem. > For example, when running the tests under > "force_parallel_mode=regress", the test failures show that there are a > lot more cases affected: > > "cannot assign TransactionIds during a parallel operation" > "cannot assign XIDs during a parallel operation" > "cannot start commands during a parallel operation" > "cannot modify commandid in active snapshot during a parallel operation" > "cannot execute nextval() during a parallel operation" > "cannot execute INSERT during a parallel operation" > "cannot execute ANALYZE during a parallel operation > "cannot update tuples during a parallel operation" > > (and there are more not currently detected by the tests, found by > searching the code). > Did you get these after applying your patch? If so, can you share the version which you are using, or if you have already posted the same then point me to the same? > As an example, with the minimal patch applied, if you had a trigger on > INSERT that, say, attempted a table creation or UPDATE/DELETE, and you > ran an "INSERT INTO ... SELECT...", it would treat the trigger > operations as being attempted in parallel-mode, and so an error would > result. > Oh, I guess this happens because you need to execute Insert in parallel-mode even though Insert is happening in the leader, right? And probably we are not facing this with "Create Table As .." because there is no trigger execution involved there. > Let me know your thoughts on how to deal with these issues. > Can you see a problem with only having parallel-mode set for scope of > Gather/GatherMerge, or do you have some other idea? > I have not thought about this yet but I don't understand your proposal. How will you set it only for the scope of Gather (Merge)? The execution of the Gather node will be interleaved with the Insert node, basically, you fetch a tuple from Gather, and then you need to Insert it. Can you be a bit more specific on what you have in mind for this? -- With Regards, Amit Kapila.
Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
On Sun, Oct 11, 2020 at 01:12:40PM -0400, Tom Lane wrote: > Noah Misch writes: > > The first attachment fixes the matter you've reported. While confirming > > that, > > I observed that gcc builds don't even use the 64-bit code in arch-ppc.h. > > Oops. The second attachment fixes that. > > I reviewed these, and tested the first one on a nearby Apple machine. > (I lack access to 64-bit PPC, so I can't actually test the second.) > They look fine, and I confirmed by examining asm output that even > the rather-old-now gcc version that Apple last shipped for PPC does > the right thing with the conditionals. Thanks for reviewing and for mentioning that old-gcc behavior. I had a comment asserting that gcc 7.2.0 didn't deduce constancy from those conditionals. Checking again now, it was just $SUBJECT preventing constancy deduction. I made the patch remove that comment. > > I plan not to back-patch either of these. > > Hmm, I'd argue for a back-patch. The issue of modern compilers > warning about the incorrect code will apply to all supported branches. > Moreover, even if we don't use these code paths today, who's to say > that someone won't back-patch a bug fix that requires them? I do not > think it's unreasonable to expect these functions to work well in > all branches that have them. Okay, I've pushed with a back-patch. compare_exchange-ppc-immediate-v1.patch affects on code generation are limited to regress.o, so it's quite safe to back-patch. I just didn't think it was standard to back-patch for the purpose of removing a -Wsign-compare warning. (Every branch is noisy under -Wsign-compare.) atomics-ppc64-gcc-v1.patch does change code generation, in the manner discussed in the big arch-ppc.h comment (starts with "This mimics gcc"). Still, I've accepted the modest risk.
Re: Resetting spilled txn statistics in pg_stat_replication
On Thu, 8 Oct 2020 at 17:59, Amit Kapila wrote: > > On Thu, Oct 8, 2020 at 1:55 PM Masahiko Sawada > wrote: > > > > On Thu, 8 Oct 2020 at 14:10, Amit Kapila wrote: > > > > > > > > > We can write if we want but there are few things we need to do for > > > that like maybe a new function like wait_for_spill_stats which will > > > check if the counters have become zero. Then probably call a reset > > > function, call a new wait function, and then again check stats to > > > ensure they are reset to 0. > > > > Yes. > > > > I am not sure if it is worth but probably it is not a bad idea > especially if we extend the existing tests based on your below idea? > > > > We can't write any advanced test which means reset the existing stats > > > perform some tests and again check stats because *slot_get_changes() > > > function can start from the previous WAL for which we have covered the > > > stats. We might write that if we can somehow track the WAL positions > > > from the previous test. I am not sure if we want to go there. > > > > Can we use pg_logical_slot_peek_changes() instead to decode the same > > transactions multiple times? > > > > I think this will do the trick. If we want to go there then I suggest > we can have a separate regression test file in test_decoding with name > as decoding_stats, stats, or something like that. We can later add the > tests related to streaming stats in that file as well. > Agreed. I've updated the patch. Please review it. Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services v10-0001-Test-stats.patch Description: Binary data
Re: Parallel INSERT (INTO ... SELECT ...)
On Mon, Oct 12, 2020 at 2:11 PM Thomas Munro wrote:
>
> On Mon, Oct 12, 2020 at 3:42 PM Greg Nancarrow wrote:
> > On Sun, Oct 11, 2020 at 1:39 PM Thomas Munro wrote:
> > > pathnode->path.total_cost += subpath->total_cost;
> > > - pathnode->path.rows += subpath->rows;
> > > + if (returningLists != NIL)
> > > + pathnode->path.rows += subpath->rows;
> > > total_size += subpath->pathtarget->width * subpath->rows;
> > > }
>
> Erm, except the condition should of course cover total_size too.
>
> > Agree, thanks (bug in existing Postgres code, right?)
>
> Yeah, I think we should go ahead and fix that up front. Here's a
> version with a commit message.
I've checked it and tested it, and it looks fine to me.
Also, it seems to align with the gripe in the old comment about width
("XXX this is totally wrong: we should report zero if no RETURNING
...").
I'm happy for you to commit it.
Regards,
Greg Nancarrow
Fujitsu Australia
Re: speed up unicode normalization quick check
On Sun, 11 Oct 2020 at 19:27, Michael Paquier wrote: > > On Thu, Oct 08, 2020 at 06:22:39PM -0400, John Naylor wrote: > > Okay, thanks. > > And applied. The following warning recently started to be shown in my environment(FreeBSD clang 8.0.1). Maybe it is relevant with this commit: unicode_norm.c:478:12: warning: implicit declaration of function 'htonl' is invalid in C99 [-Wimplicit-function-declaration] hashkey = htonl(ch); ^ 1 warning generated. Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Resetting spilled txn statistics in pg_stat_replication
On Thu, 8 Oct 2020 at 22:57, Amit Kapila wrote: > > On Thu, Oct 8, 2020 at 7:46 AM Masahiko Sawada > wrote: > > > > On Wed, 7 Oct 2020 at 17:52, Amit Kapila wrote: > > > > > > > > I think after we are done with this the next > > > step would be to finish the streaming stats work [1]. We probably need > > > to review and add the test case in that patch. If nobody else shows up > > > I will pick it up and complete it. > > > > +1 > > I can review that patch. > > > > I have rebased the stream stats patch and made minor modifications. I > haven't done a detailed review but one thing that I think is not > correct is: > @@ -3496,10 +3499,18 @@ ReorderBufferStreamTXN(ReorderBuffer *rb, > ReorderBufferTXN *txn) > txn->snapshot_now = NULL; > } > > + > + rb->streamCount += 1; > + rb->streamBytes += txn->total_size; > + > + /* Don't consider already streamed transaction. */ > + rb->streamTxns += (rbtxn_is_streamed(txn)) ? 0 : 1; > + > /* Process and send the changes to output plugin. */ > ReorderBufferProcessTXN(rb, txn, InvalidXLogRecPtr, snapshot_now, > command_id, true); > > I think we should update the stream stats after > ReorderBufferProcessTXN rather than before because any error in > ReorderBufferProcessTXN can lead to an unnecessary update of stats. > But OTOH, the txn flags, and other data can be changed after > ReorderBufferProcessTXN so we need to save them in a temporary > variable before calling the function. Thank you for updating the patch! I've not looked at the patch in-depth yet but RBTXN_IS_STREAMED could be cleared after ReorderBUfferProcessTXN()? BTW maybe it's better to start a new thread for this patch as the title is no longer relevant. Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: speed up unicode normalization quick check
On Mon, Oct 12, 2020 at 02:43:06PM +0900, Masahiko Sawada wrote: > The following warning recently started to be shown in my > environment(FreeBSD clang 8.0.1). Maybe it is relevant with this > commit: > > unicode_norm.c:478:12: warning: implicit declaration of function > 'htonl' is invalid in C99 [-Wimplicit-function-declaration] > hashkey = htonl(ch); > ^ Thanks, it is of course relevant to this commit. None of the BSD animals complain here. So, while it would be tempting to have an extra include with arpa/inet.h, I think that it would be better to just use pg_hton32() in pg_bswap.h, as per the attached. Does that take care of your problem? -- Michael diff --git a/src/common/unicode_norm.c b/src/common/unicode_norm.c index 626645ac87..4bb6a0f587 100644 --- a/src/common/unicode_norm.c +++ b/src/common/unicode_norm.c @@ -23,6 +23,7 @@ #ifndef FRONTEND #include "common/unicode_normprops_table.h" #endif +#include "port/pg_bswap.h" #ifndef FRONTEND #define ALLOC(size) palloc(size) @@ -475,7 +476,7 @@ qc_hash_lookup(pg_wchar ch, const pg_unicode_norminfo *norminfo) * Compute the hash function. The hash key is the codepoint with the bytes * in network order. */ - hashkey = htonl(ch); + hashkey = pg_hton32(ch); h = norminfo->hash(&hashkey); /* An out-of-range result implies no match */ signature.asc Description: PGP signature
Re: Parallel INSERT (INTO ... SELECT ...)
On Mon, Oct 12, 2020 at 9:01 AM Amit Kapila wrote: > > On Mon, Oct 12, 2020 at 6:51 AM Greg Nancarrow wrote: > > > > > Let me know your thoughts on how to deal with these issues. > > Can you see a problem with only having parallel-mode set for scope of > > Gather/GatherMerge, or do you have some other idea? > > > > I have not thought about this yet but I don't understand your > proposal. How will you set it only for the scope of Gather (Merge)? > The execution of the Gather node will be interleaved with the Insert > node, basically, you fetch a tuple from Gather, and then you need to > Insert it. Can you be a bit more specific on what you have in mind for > this? > One more thing I would like to add here is that we can't exit parallel-mode till the workers are running (performing the scan or other operation it is assigned with) and shared memory is not destroyed. Otherwise, the leader can perform un-safe things like assigning new commandsids or probably workers can send some error for which the leader should still be in parallel-mode. So, considering this I think we need quite similar checks (similar to parallel inserts) to make even the Select part parallel for Inserts. If we do that then you won't face many of the problems you mentioned above like executing triggers that contain parallel-unsafe stuff. I feel still it will be beneficial to do this as it will cover cases like Insert with GatherMerge underneath it which would otherwise not possible. -- With Regards, Amit Kapila.
Re: speed up unicode normalization quick check
On Mon, 12 Oct 2020 at 15:27, Michael Paquier wrote: > > On Mon, Oct 12, 2020 at 02:43:06PM +0900, Masahiko Sawada wrote: > > The following warning recently started to be shown in my > > environment(FreeBSD clang 8.0.1). Maybe it is relevant with this > > commit: > > > > unicode_norm.c:478:12: warning: implicit declaration of function > > 'htonl' is invalid in C99 [-Wimplicit-function-declaration] > > hashkey = htonl(ch); > > ^ > > Thanks, it is of course relevant to this commit. None of the > BSD animals complain here. So, while it would be tempting to have an > extra include with arpa/inet.h, I think that it would be better to > just use pg_hton32() in pg_bswap.h, as per the attached. Does that > take care of your problem? Thank you for the patch! Yes, this patch resolves the problem. Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
