Hi,
On 2020-06-16 15:20:11 -0400, Alvaro Herrera wrote:
> On 2020-Jun-15, Andres Freund wrote:
>
> > > Also, I wonder if someone would be willing to set up a BF animal for this.
> >
> > FWIW, I've requested a buildfarm animal id for this a few days ago, but
> > haven't received a response yet...
Hi,
On 2020-06-18 12:29:40 -0400, Tom Lane wrote:
> Robert Haas writes:
> > On Thu, Jun 18, 2020 at 11:59 AM Tom Lane wrote:
> >> Sure, but wouldn't making the SpinLockAcquire layer into static inlines be
> >> sufficient to address that point, with no need to touch s_lock.h at all?
>
> > I mean
Robert Haas writes:
> On Thu, Jun 18, 2020 at 11:59 AM Tom Lane wrote:
>> Sure, but wouldn't making the SpinLockAcquire layer into static inlines be
>> sufficient to address that point, with no need to touch s_lock.h at all?
> I mean, wouldn't you then end up with a bunch of 1-line functions
> w
On Thu, Jun 18, 2020 at 11:59 AM Tom Lane wrote:
> Sure, but wouldn't making the SpinLockAcquire layer into static inlines be
> sufficient to address that point, with no need to touch s_lock.h at all?
I mean, wouldn't you then end up with a bunch of 1-line functions
where you can step into the fu
Robert Haas writes:
> On Wed, Jun 17, 2020 at 5:29 PM Tom Lane wrote:
>> I wouldn't object to making the outer-layer macros in spin.h into static
>> inlines; as mentioned, that might have some debugging benefits. But I
>> think messing with s_lock.h for marginal cosmetic reasons is a foolish
>>
On Wed, Jun 17, 2020 at 5:29 PM Tom Lane wrote:
> See the "Default Definitions", down near the end.
Oh, yeah. I didn't realize you meant just inside this file itself.
That is slightly awkward. I initially thought there was no problem
because there seem to be no remaining non-default definitions o
Robert Haas writes:
> On Wed, Jun 17, 2020 at 3:45 PM Tom Lane wrote:
>> The macros are kind of necessary unless you want to make s_lock.h
>> a bunch messier, because we use #ifdef tests on them.
> Where?
See the "Default Definitions", down near the end.
>> We could get rid of the double layer
On Wed, Jun 17, 2020 at 3:45 PM Tom Lane wrote:
> Robert Haas writes:
> > This seems like it's straight out of the department of pointless
> > abstraction layers. Maybe we should remove all of the S_WHATEVER()
> > stuff and just define SpinLockAcquire() where we currently define
> > S_LOCK(), Spi
Robert Haas writes:
> This seems like it's straight out of the department of pointless
> abstraction layers. Maybe we should remove all of the S_WHATEVER()
> stuff and just define SpinLockAcquire() where we currently define
> S_LOCK(), SpinLockRelease() where we currently define S_UNLOCK(), etc.
>
On Wed, Jun 17, 2020 at 2:33 PM Andres Freund wrote:
> > It seems odd and confusing that we have both
> > S_LOCK() and s_lock(), anyway. Differentiating functions based on case
> > is not great practice.
>
> It's a terrible idea, yes. Since we don't actually have any non-default
> implementation
Hi,
On 2020-06-17 10:34:31 -0400, Robert Haas wrote:
> On Tue, Jun 16, 2020 at 3:28 PM Andres Freund wrote:
> > > I think 0003 looks a little strange: it seems to be
> > > testing things that might be implementation details of other things,
> > > and I'm not sure that's really correct. In particu
On Tue, Jun 16, 2020 at 3:28 PM Andres Freund wrote:
> > I think 0003 looks a little strange: it seems to be
> > testing things that might be implementation details of other things,
> > and I'm not sure that's really correct. In particular:
>
> My main motivation was to have something that runs mo
Hi,
On 2020-06-16 14:59:19 -0400, Robert Haas wrote:
> On Mon, Jun 15, 2020 at 9:37 PM Andres Freund wrote:
> > What do you think about 0002?
> >
> > With regard to the cost of the expensive test in 0003, I'm somewhat
> > inclined to add that to the buildfarm for a few days and see how it
> > act
On 2020-Jun-15, Andres Freund wrote:
> > Also, I wonder if someone would be willing to set up a BF animal for this.
>
> FWIW, I've requested a buildfarm animal id for this a few days ago, but
> haven't received a response yet...
I did send it out, with name rorqual -- didn't you get that? Will
On Mon, Jun 15, 2020 at 9:37 PM Andres Freund wrote:
> What do you think about 0002?
>
> With regard to the cost of the expensive test in 0003, I'm somewhat
> inclined to add that to the buildfarm for a few days and see how it
> actually affects the few bf animals without atomics. We can rip it ou
Hi,
On 2020-06-09 17:04:42 -0400, Robert Haas wrote:
> On Tue, Jun 9, 2020 at 3:37 PM Andres Freund wrote:
> > Hm. Looking at this again, perhaps the better fix would be to simply not
> > look at the concrete values of the barrier inside the signal handler?
> > E.g. we could have a new PROCSIG_GL
On Thu, Jun 11, 2020 at 1:26 PM Andres Freund wrote:
> Well, pss_barrierCheckMask member is just 32bit, so it seems odd to
> declare the local variable 64bit?
>
> uint64 flagbit = UINT64CONST(1) << (uint64) type;
> ...
> pg_atomic_fetch_or_u32(&slot->pss_barrierChe
Hi,
On 2020-06-10 13:37:59 -0400, Robert Haas wrote:
> On Tue, Jun 9, 2020 at 6:54 PM Andres Freund wrote:
> > What do you think about my idea of having a BEGIN/END_SIGNAL_HANDLER?
> > That'd make it much easier to write assertions forbidding palloc, 64bit
> > atomics, ...
>
> I must have missed
Hi,
On 2020-06-10 07:26:32 -0400, Robert Haas wrote:
> On Fri, Jun 5, 2020 at 8:19 PM Andres Freund wrote:
> > Randomly noticed while looking at the code:
> > uint64 flagbit = UINT64CONST(1) << (uint64) type;
> >
> > that shouldn't be 64bit, right?
>
> I'm going to admit ignoran
On Tue, Jun 9, 2020 at 6:54 PM Andres Freund wrote:
> What do you think about my idea of having a BEGIN/END_SIGNAL_HANDLER?
> That'd make it much easier to write assertions forbidding palloc, 64bit
> atomics, ...
I must have missed the previous place where you suggested this, but I
think it's a g
Robert Haas writes:
> On Fri, Jun 5, 2020 at 8:19 PM Andres Freund wrote:
>> Randomly noticed while looking at the code:
>> uint64 flagbit = UINT64CONST(1) << (uint64) type;
>>
>> that shouldn't be 64bit, right?
> I'm going to admit ignorance here. What's the proper coding rule?
On Fri, Jun 5, 2020 at 8:19 PM Andres Freund wrote:
> Randomly noticed while looking at the code:
> uint64 flagbit = UINT64CONST(1) << (uint64) type;
>
> that shouldn't be 64bit, right?
I'm going to admit ignorance here. What's the proper coding rule?
--
Robert Haas
EnterpriseD
Hi,
On 2020-06-09 17:04:42 -0400, Robert Haas wrote:
> On Tue, Jun 9, 2020 at 3:37 PM Andres Freund wrote:
> > Hm. Looking at this again, perhaps the better fix would be to simply not
> > look at the concrete values of the barrier inside the signal handler?
> > E.g. we could have a new PROCSIG_GL
On Tue, Jun 9, 2020 at 3:37 PM Andres Freund wrote:
> Hm. Looking at this again, perhaps the better fix would be to simply not
> look at the concrete values of the barrier inside the signal handler?
> E.g. we could have a new PROCSIG_GLOBAL_BARRIER, which just triggers
> ProcSignalBarrierPending t
Hi,
On 2020-06-08 23:08:47 -0700, Andres Freund wrote:
> On 2020-06-05 17:19:26 -0700, Andres Freund wrote:
> > I wrote a patch for this, and when I got around to to testing it, I
> > found that our tests currently don't pass when using both
> > --disable-spinlocks and --disable-atomics. Turns out
On 2020-06-05 17:19:26 -0700, Andres Freund wrote:
> Hi,
>
> On 2020-06-04 19:33:02 -0700, Andres Freund wrote:
> > But it looks like that code is currently buggy (and looks like it always
> > has been), because we don't look at the nested argument when
> > initializing the semaphore. So we curren
Hi,
On 2020-06-05 22:52:47 -0400, Tom Lane wrote:
> Andres Freund writes:
> > On 2020-06-05 21:01:56 -0400, Tom Lane wrote:
> >> At some point I think we'll have to give up --disable-spinlocks; it's
> >> really of pretty marginal use (how often does anyone port PG to a new
> >> CPU type?) and the
Andres Freund writes:
> On 2020-06-05 21:01:56 -0400, Tom Lane wrote:
>> At some point I think we'll have to give up --disable-spinlocks; it's
>> really of pretty marginal use (how often does anyone port PG to a new
>> CPU type?) and the number of weird interactions it adds in this area
>> seems l
Hi,
On 2020-06-05 21:01:56 -0400, Tom Lane wrote:
> > I'm not really sure what to do about that issue. The easisest thing
> > would probably be to change the barrier generation to 32bit (which
> > doesn't have to use locks for reads in any situation).
>
> Yeah, I think we need a hard rule that you
Andres Freund writes:
> I wrote a patch for this, and when I got around to to testing it, I
> found that our tests currently don't pass when using both
> --disable-spinlocks and --disable-atomics. Turns out to not be related
> to the issue above, but the global barrier support added in 13.
> That
Hi,
On 2020-06-04 19:33:02 -0700, Andres Freund wrote:
> But it looks like that code is currently buggy (and looks like it always
> has been), because we don't look at the nested argument when
> initializing the semaphore. So we currently allocate too many
> semaphores, without benefiting from th
Hi,
On 2020-06-04 15:13:29 -0400, Tom Lane wrote:
> Andres Freund writes:
> > On 2020-06-04 14:50:40 -0400, Tom Lane wrote:
> >> 2. The computed completePasses value would go backwards. I bet
> >> that wouldn't matter too much either, or at least we could teach
> >> BgBufferSync to cope. (I not
Hi,
On 2020-06-04 15:07:34 -0400, Tom Lane wrote:
> Andres Freund writes:
> > I'd still like to know which problem we're actually trying to solve
> > here. I don't understand the "error" issues you mentioned upthread.
>
> If you error out of getting the inner spinlock, the outer spinlock
> is stu
Andres Freund writes:
> On 2020-06-04 14:50:40 -0400, Tom Lane wrote:
>> 2. The computed completePasses value would go backwards. I bet
>> that wouldn't matter too much either, or at least we could teach
>> BgBufferSync to cope. (I notice the comments therein suggest that
>> it is already design
Andres Freund writes:
> I'd still like to know which problem we're actually trying to solve
> here. I don't understand the "error" issues you mentioned upthread.
If you error out of getting the inner spinlock, the outer spinlock
is stuck, permanently, because there is no mechanism for spinlock
re
Hi,
On 2020-06-04 14:50:40 -0400, Tom Lane wrote:
> Actually ... we could probably use this design with a uint32 counter
> as well, on machines where the 64-bit operations would be slow.
On skylake-x even a 32bit [i]div is still 26 cycles. That's more than an
atomic operation 18 cycles.
> 2. Th
Hi,
On 2020-06-04 13:57:19 -0400, Tom Lane wrote:
> Andres Freund writes:
> > On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
> >> This seems to me to be very bad code.
>
> > I think straight out prohibiting use of atomics inside a spinlock will
> > lead to more complicated and slower code, rather
I wrote:
> I think a good case could be made for ripping out what's there now
> and just redefining nextVictimBuffer as a pg_atomic_uint64 that we
> never reset (ie, make its comment actually true). Then ClockSweepTick
> reduces to
> pg_atomic_fetch_add_u64(&StrategyControl->nextVictimBuffer, 1) %
Andres Freund writes:
> On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
>> This seems to me to be very bad code.
> I think straight out prohibiting use of atomics inside a spinlock will
> lead to more complicated and slower code, rather than than improving
> anything. I'm to blame for the ClockSwee
On Thu, Jun 04, 2020 at 09:40:31AM +1200, Thomas Munro wrote:
> Yeah. It'd be fine to move that after the spinlock release. Although
> it's really just for informational purposes only, not for any data
> integrity purpose, reading it before the spinlock acquisition would
> theoretically allow it
On Thu, Jun 4, 2020 at 8:45 AM Andres Freund wrote:
> On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
> > In the second place, this coding seems to me to indicate serious
> > confusion about which lock is protecting what. In the above example,
> > if writtenUpto is only accessed through atomic oper
Hi,
On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
> In connection with the nearby thread about spinlock coding rule
> violations, I noticed that we have several places that are doing
> things like this:
>
> SpinLockAcquire(&WalRcv->mutex);
> ...
> written_lsn = pg_atomic_read_u64
In connection with the nearby thread about spinlock coding rule
violations, I noticed that we have several places that are doing
things like this:
SpinLockAcquire(&WalRcv->mutex);
...
written_lsn = pg_atomic_read_u64(&WalRcv->writtenUpto);
...
SpinLockReleas
43 matches
Mail list logo
