Re: tablesync patch broke the assumption that logical rep depends on?

On 21/04/17 16:31, Petr Jelinek wrote:
> On 21/04/17 16:23, Peter Eisentraut wrote:
>> On 4/21/17 10:11, Petr Jelinek wrote:
>>> On 21/04/17 16:09, Peter Eisentraut wrote:
>>>> On 4/20/17 14:29, Petr Jelinek wrote:
>>>>> + /* Find unused worker slot. */
>>>>> + if (!w->in_use)
>>>>> {
>>>>> - worker = &LogicalRepCtx->workers[slot];
>>>>> - break;
>>>>> + worker = w;
>>>>> + slot = i;
>>>>> + }
>>>>
>>>> Doesn't this still need a break? Otherwise it always picks the last slot.
>>>>
>>>
>>> Yes it will pick the last slot, does that matter though, is the first
>>> one better somehow?
>>>
>>> We can't break because we also need to continue the counter (I think the
>>> issue that the counter solves is probably just theoretical, but still).
>>
>> I see. I think the code would be less confusing if we break the loop
>> like before and call logicalrep_sync_worker_count() separately.
>>
>>> Hmm actually, maybe the if (!w->in_use) should be if (worker == NULL &&
>>> !w->in_use)?
>>
>> That would also do it. But it's getting a bit fiddly.
>>
>
> I just wanted to avoid looping twice, especially since the garbage
> collecting code has to also do the loop. I guess I'll go with my
> original coding for this then which was to put retry label above the
> loop first, then try finding worker slot, if found call the
> logicalrep_sync_worker_count and if not found do the garbage collection
> and if we cleaned up something then goto retry.
>

Here is the patch doing just that.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services