chore: event_payload column can be JSONB or TEXT #5372
Conversation
Sidddddarth
force-pushed
the
chore.jobsdbPayloadColumnType
branch
from
c053da4 to
c5c21e3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5372 +/- ##
==========================================
+ Coverage 74.82% 74.85% +0.02%
==========================================
Files 438 438
Lines 61331 61419 +88
==========================================
+ Hits 45893 45974 +81
- Misses 12895 12909 +14
+ Partials 2543 2536 -7 ☔ View full report in Codecov by Sentry. |
Sidddddarth
changed the title
jobsdb/jobsdb.go
Outdated
| @@ -770,6 +798,10 @@ func (jd *Handle) init() { | |||
| jd.config = config.Default | |||
| } | |||
|
|
|||
| if jd.conf.payloadColumnType == 0 { | |||
| jd.conf.payloadColumnType = payloadColumnType(jd.config.GetIntVar(2, 1, "JobsDB.payloadColumnType")) | |||
There was a problem hiding this comment.
| jd.conf.payloadColumnType = payloadColumnType(jd.config.GetIntVar(2, 1, "JobsDB.payloadColumnType")) | |
| jd.conf.payloadColumnType = payloadColumnType(jd.config.GetIntVar(int(TEXT), 1, "JobsDB.payloadColumnType")) |
There was a problem hiding this comment.
Why do we want TEXT to be default?
There was a problem hiding this comment.
because it's faster than using jsonb column
saw no reason to not change the default to text
Sidddddarth
force-pushed
the
chore.jobsdbPayloadColumnType
branch
from
f964bf8 to
5a4adff
Compare
jobsdb/integration_test.go
Outdated
| jobDB.TearDown() | ||
|
|
||
| conf.Set("JobsDB.payloadColumnType", 2) | ||
| textDB := &Handle{config: conf} | ||
| require.NoError(t, textDB.Setup(ReadWrite, true, strings.ToLower(rand.String(5)))) |
There was a problem hiding this comment.
Do you think we should use a table-driven test to avoid repetition here?
Run same test for payloadColumnType: [1,2,3].
Also, payloadColumnType could be better. (separate comment)
There was a problem hiding this comment.
done - incorporating fuzz tests is pending
jobsdb/jobsdb.go
Outdated
| if jd.conf.payloadColumnType == 0 { | ||
| jd.conf.payloadColumnType = payloadColumnType(jd.config.GetIntVar(int(TEXT), 1, "JobsDB.payloadColumnType")) | ||
| } |
There was a problem hiding this comment.
I believe it is preferable to configure this as a text, using the original Postgres types to avoid any confusion.
There was a problem hiding this comment.
Also, shouldn't the default be JSONB ?
There was a problem hiding this comment.
yeah will make the config change, makes sense
as for the default - I've just used the new TEXT column because it's faster..
No reason to not use it right..? We'll have to change all environments to TEXT at some point.
| @@ -459,18 +459,85 @@ func (jd *Handle) getMigrationList(dsList []dataSetT) (migrateFrom []dsWithPendi | |||
| return | |||
| } | |||
|
|
|||
| func getColumnConversion(srcType, destType string) string { | |||
There was a problem hiding this comment.
Is it possible to find a universal way to do the mapping, that will always work?
Sidddddarth
force-pushed
the
chore.jobsdbPayloadColumnType
branch
from
e5e9442 to
b55615a
Compare
Description
event_payloadcolumn in newly created jobs tables will beTEXTcolumns.Allows migrations from one type to the other, so changing from one to the other shouldn't fail. Changing back to
JSONBwould allow server processes to continue, however utility postgres functions likeunionjobsdbwould fail as the function definition returns table with different column types.At startup(for
w,rwjobsdbs) - we either update the last DS's event_payload column if empty or create a new dataset if the last has some jobs already - both provided the event type column of the last ds is different from what's configured.Not a reloadable selection.
Allowed creating
BYTEAcolumns but only for benchmark purposes.TEXTseems to be doing better and also avoids other character validation problems.Linear Ticket
Resolves PIPE-1649
Security